From ebae3e8827f9605858717875a51da82231e5f19e Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 21:52:42 +0400 Subject: [PATCH 01/13] fix(poll): let the reconnect backoff escalate and run its full delay A host that authenticates but cannot answer the metric commands - a network appliance with no shell - failed every cycle, and the reset on a successful connect pinned it to the first backoff step: one login every 30 s, forever. Reset on a cycle that produced data instead, and stop letting a refresh signal cut the backoff wait short, which let the GUI's refresh timer retry a failing host every few seconds. --- crates/omnyssh-core/src/ssh/pool.rs | 62 +++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 3 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/pool.rs b/crates/omnyssh-core/src/ssh/pool.rs index 3a272d8..9da11c3 100644 --- a/crates/omnyssh-core/src/ssh/pool.rs +++ b/crates/omnyssh-core/src/ssh/pool.rs @@ -124,7 +124,6 @@ async fn run_host_poller( send_status(&tx, &host.name, ConnectionStatus::Connecting).await; match SshSession::connect(&host).await { Ok(s) => { - backoff.reset(); send_status(&tx, &host.name, ConnectionStatus::Connected).await; session = Some(s); discovery_done = false; // Reset discovery flag on new connection @@ -134,7 +133,7 @@ async fn run_host_poller( send_status(&tx, &host.name, ConnectionStatus::Failed(e.to_string())).await; // Wait with backoff, allowing early refresh. let delay = backoff.next_delay(); - wait_or_refresh(delay, &mut refresh_rx).await; + wait_backoff(delay, &mut refresh_rx).await; continue; } } @@ -180,6 +179,10 @@ async fn run_host_poller( let sess = session.as_ref().expect("session is Some here"); match collect_metrics(sess, &host.name).await { Ok(metrics) => { + // A cycle that produced data proves the host is pollable. Resetting + // on connect instead pins a host that authenticates but cannot run + // commands (a network appliance) to the first backoff step forever. + backoff.reset(); if tx .send(CoreEvent::MetricsUpdate(host.name.clone(), metrics)) .await @@ -194,7 +197,7 @@ async fn run_host_poller( session.take(); send_status(&tx, &host.name, ConnectionStatus::Failed(e.to_string())).await; let delay = backoff.next_delay(); - wait_or_refresh(delay, &mut refresh_rx).await; + wait_backoff(delay, &mut refresh_rx).await; continue; } } @@ -212,6 +215,25 @@ async fn wait_or_refresh(delay: Duration, refresh_rx: &mut mpsc::Receiver<()>) { } } +/// Sleep the whole `delay`, discarding refresh signals. +/// +/// A reconnect must never dial faster than the backoff schedule: the GUI drives +/// `refresh_all` on its own timer, which is indistinguishable from a keypress +/// here and would otherwise retry a failing host every few seconds. +async fn wait_backoff(delay: Duration, refresh_rx: &mut mpsc::Receiver<()>) { + let deadline = Instant::now() + delay; + loop { + let left = deadline.saturating_duration_since(Instant::now()); + if left.is_zero() { + return; + } + tokio::select! { + _ = tokio::time::sleep(left) => return, + _ = refresh_rx.recv() => {} + } + } +} + async fn send_status(tx: &mpsc::Sender, name: &str, status: ConnectionStatus) { let _ = tx .send(CoreEvent::HostStatusChanged(name.to_string(), status)) @@ -397,6 +419,40 @@ async fn parse_ram_combined(mem_out: &str, session: &SshSession) -> Option mod tests { use super::*; + #[test] + fn backoff_escalates_and_only_a_reset_returns_it_to_the_first_step() { + let mut backoff = BackoffState::new(); + let steps: Vec = (0..5).map(|_| backoff.next_delay().as_secs()).collect(); + assert_eq!(steps, vec![30, 60, 120, 300, 300]); + + backoff.reset(); + assert_eq!(backoff.next_delay().as_secs(), 30); + } + + #[tokio::test(start_paused = true)] + async fn wait_backoff_ignores_refresh_signals() { + let (tx, mut rx) = mpsc::channel::<()>(4); + for _ in 0..4 { + tx.try_send(()).expect("channel has room"); + } + + let start = tokio::time::Instant::now(); + wait_backoff(Duration::from_secs(300), &mut rx).await; + + assert_eq!(start.elapsed(), Duration::from_secs(300)); + } + + #[tokio::test(start_paused = true)] + async fn wait_or_refresh_still_returns_early() { + let (tx, mut rx) = mpsc::channel::<()>(4); + tx.try_send(()).expect("channel has room"); + + let start = tokio::time::Instant::now(); + wait_or_refresh(Duration::from_secs(300), &mut rx).await; + + assert!(start.elapsed() < Duration::from_secs(1)); + } + #[test] fn top_processes_command_excludes_monitor_pid_chain() { let cmd = top_processes_command("-eo pid=,ppid=,pcpu=,pmem=,comm= --sort=-pcpu"); From f34a42003dd536951270406cbc8c8c8aa968b696 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 21:52:49 +0400 Subject: [PATCH 02/13] fix(ssh): keep a session whose peer ignores keepalives russh skips resetting the inactivity timer on the iteration that sends a keepalive, so a peer that never answers keepalive@openssh.com was disconnected after 30 s even while its commands still ran - and the poller then re-logged in on every cycle. Drop the inactivity timeout; keepalive_max still bounds a dead peer. --- crates/omnyssh-core/src/ssh/session.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/omnyssh-core/src/ssh/session.rs b/crates/omnyssh-core/src/ssh/session.rs index a1d5364..917627f 100644 --- a/crates/omnyssh-core/src/ssh/session.rs +++ b/crates/omnyssh-core/src/ssh/session.rs @@ -309,7 +309,10 @@ pub(crate) async fn connect_budget(host: &Host) -> Duration { /// The shared russh client configuration (timeouts + keepalives). fn client_config() -> Arc { Arc::new(client::Config { - inactivity_timeout: Some(Duration::from_secs(30)), + // No inactivity timeout: russh skips resetting it on the iteration that + // sends a keepalive, so a peer that never answers `keepalive@openssh.com` + // (common in appliance SSH stacks) was torn down after 30 s even while + // its commands still ran. Liveness stays bounded by `keepalive_max`. keepalive_interval: Some(Duration::from_secs(15)), keepalive_max: 3, ..Default::default() From 32e8dcd6d7a4e275dd17eccc3f7337281d0407c2 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 21:54:32 +0400 Subject: [PATCH 03/13] fix(metrics): pin the locale for the parsed monitoring commands On a server whose system language uses a comma decimal separator, top prints "99,1 id". The CPU parser splits that line on ',', read the tenths digit as the idle value and reported 100 minus it - a near-constant 91-100 % on an idle machine - while free and df translate their labels and left RAM and Disk at N/A. Run the parsed commands under LC_ALL=C, and LC_NUMERIC/LC_MESSAGES only for the ps pipeline, whose output reaches the user and must keep the host's LC_CTYPE. The two CPU parsers also normalise a decimal comma themselves, so a host without env can no longer turn a comma into a plausible-looking percentage. --- crates/omnyssh-core/src/ssh/metrics.rs | 27 ++++++++++ crates/omnyssh-core/src/ssh/pool.rs | 55 ++++++++++++++++----- crates/omnyssh-core/tests/metrics_parser.rs | 43 ++++++++++++++++ 3 files changed, 114 insertions(+), 11 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/metrics.rs b/crates/omnyssh-core/src/ssh/metrics.rs index 39187b9..f6a083d 100644 --- a/crates/omnyssh-core/src/ssh/metrics.rs +++ b/crates/omnyssh-core/src/ssh/metrics.rs @@ -87,7 +87,33 @@ fn parse_cpu_busybox(line: &str) -> Option { None } +/// Rewrites a decimal comma as a decimal point. +/// +/// The commands are run under a pinned locale, but that is best-effort: a host +/// without `env`, or a wrapper that re-exports a locale, still reaches the +/// parsers with `99,1 id`. Only a comma between two digits is a radix point — +/// every other comma separates fields and must survive. +fn normalize_decimal_commas(line: &str) -> String { + let chars: Vec = line.chars().collect(); + chars + .iter() + .enumerate() + .map(|(i, &c)| { + let radix = c == ',' + && i > 0 + && chars[i - 1].is_ascii_digit() + && chars.get(i + 1).is_some_and(char::is_ascii_digit); + if radix { + '.' + } else { + c + } + }) + .collect() +} + fn parse_cpu_linux_top_line(line: &str) -> Option { + let line = normalize_decimal_commas(line); // Strip leading label (everything up to and including the colon). let after_colon = line.split_once(':')?.1; @@ -119,6 +145,7 @@ pub fn parse_cpu_top_macos(output: &str) -> Option { for line in output.lines() { let lower = line.trim().to_lowercase(); if lower.starts_with("cpu usage:") { + let line = normalize_decimal_commas(line); // Find "idle" value for part in line.split(',') { let part = part.trim(); diff --git a/crates/omnyssh-core/src/ssh/pool.rs b/crates/omnyssh-core/src/ssh/pool.rs index 9da11c3..e816a74 100644 --- a/crates/omnyssh-core/src/ssh/pool.rs +++ b/crates/omnyssh-core/src/ssh/pool.rs @@ -30,6 +30,23 @@ use crate::ssh::session::SshSession; const BACKOFF_SECS: [u64; 4] = [30, 60, 120, 300]; +// --------------------------------------------------------------------------- +// Metric commands +// --------------------------------------------------------------------------- + +// Metric output is machine-parsed, so the locale has to be pinned: a server set +// to a comma-decimal language prints "99,1 id" and "Speicher:", which the parsers +// read as garbage or not at all. `env` rather than a `VAR=value cmd` prefix, which +// is not valid csh/tcsh syntax. +const CPU_CMD: &str = "env LC_ALL=C top -bn1 2>/dev/null | head -5"; +const MEM_CMD: &str = "env LC_ALL=C free -b 2>/dev/null || env LC_ALL=C vm_stat 2>/dev/null"; +const DISK_CMD: &str = "env LC_ALL=C df -k / 2>/dev/null"; +const UPTIME_CMD: &str = "env LC_ALL=C uptime 2>/dev/null"; +const CPU_MACOS_CMD: &str = "env LC_ALL=C top -l 1 -n 0 2>/dev/null | grep 'CPU usage'"; +// `ps` output reaches the user, so keep the host's LC_CTYPE: under a full +// `LC_ALL=C` GNU ps replaces every non-ASCII byte of a process name with '?'. +const PS_LOCALE: &str = "env LC_NUMERIC=C LC_MESSAGES=C"; + struct BackoffState { step: usize, } @@ -255,10 +272,10 @@ async fn send_status(tx: &mpsc::Sender, name: &str, status: Connectio async fn collect_metrics(session: &SshSession, host_name: &str) -> anyhow::Result { // Run all commands concurrently for speed. let (cpu_out, mem_out, disk_out, uptime_out, loadavg_out) = tokio::join!( - session.run_command("top -bn1 2>/dev/null | head -5"), - session.run_command("free -b 2>/dev/null || vm_stat 2>/dev/null"), - session.run_command("df -k / 2>/dev/null"), - session.run_command("uptime 2>/dev/null"), + session.run_command(CPU_CMD), + session.run_command(MEM_CMD), + session.run_command(DISK_CMD), + session.run_command(UPTIME_CMD), session.run_command("cat /proc/loadavg 2>/dev/null"), ); @@ -369,8 +386,8 @@ async fn collect_top_processes(session: &SshSession) -> Option> /// "process data unavailable". fn top_processes_command(ps_args: &str) -> String { format!( - "g=$(ps -o ppid= -p $PPID 2>/dev/null | tr -d ' '); \ - ps {ps_args} 2>/dev/null | \ + "g=$({PS_LOCALE} ps -o ppid= -p $PPID 2>/dev/null | tr -d ' '); \ + {PS_LOCALE} ps {ps_args} 2>/dev/null | \ awk -v s=$$ -v p=$PPID -v g=\"$g\" \ '$1!=s && $1!=p && $1!=g && $2!=s && $2!=p \ {{$1=\"\";$2=\"\";sub(/^[ \\t]+/,\"\");print}}' | \ @@ -384,10 +401,7 @@ async fn parse_cpu_combined(top_out: &str, session: &SshSession) -> Option return Some(v); } // Try macOS top format. - let macos_out = session - .run_command("top -l 1 -n 0 2>/dev/null | grep 'CPU usage'") - .await - .unwrap_or_default(); + let macos_out = session.run_command(CPU_MACOS_CMD).await.unwrap_or_default(); if let Some(v) = parse_cpu_top_macos(&macos_out) { return Some(v); } @@ -453,12 +467,31 @@ mod tests { assert!(start.elapsed() < Duration::from_secs(1)); } + #[test] + fn metric_commands_pin_the_locale() { + for cmd in [CPU_CMD, MEM_CMD, DISK_CMD, UPTIME_CMD, CPU_MACOS_CMD] { + assert!(cmd.starts_with("env LC_ALL=C "), "unpinned command: {cmd}"); + } + // The `||` fallback needs the prefix on both sides. + assert_eq!(MEM_CMD.matches("env LC_ALL=C ").count(), 2); + } + + #[test] + fn top_processes_command_pins_numbers_but_keeps_the_host_ctype() { + let cmd = top_processes_command("-eo pcpu="); + + // Both `ps` invocations are pinned, so a comma-decimal host still parses. + assert_eq!(cmd.matches(PS_LOCALE).count(), 2); + // LC_CTYPE stays with the host: process names reach the user verbatim. + assert!(!cmd.contains("LC_ALL")); + } + #[test] fn top_processes_command_excludes_monitor_pid_chain() { let cmd = top_processes_command("-eo pid=,ppid=,pcpu=,pmem=,comm= --sort=-pcpu"); // The grandparent PID is resolved before the pipeline runs. - assert!(cmd.contains("g=$(ps -o ppid= -p $PPID")); + assert!(cmd.contains("ps -o ppid= -p $PPID")); // The awk filter binds the shell, its parent sshd and the grandparent. assert!(cmd.contains("-v s=$$")); assert!(cmd.contains("-v p=$PPID")); diff --git a/crates/omnyssh-core/tests/metrics_parser.rs b/crates/omnyssh-core/tests/metrics_parser.rs index 4d12dbc..4b5928a 100644 --- a/crates/omnyssh-core/tests/metrics_parser.rs +++ b/crates/omnyssh-core/tests/metrics_parser.rs @@ -34,6 +34,49 @@ fn cpu_alpine_busybox() { assert!((pct - 6.0).abs() < 0.2, "expected ~6.0, got {pct}"); } +// A server whose system language uses a comma decimal separator prints +// "99,1 id"; splitting the line on ',' then read the tenths digit as the idle +// value and reported 100 - that, i.e. ~91-100 % on an idle machine. +#[test] +fn cpu_comma_decimal_locale() { + let out = "%CPU(s): 0,0 us, 0,9 sy, 0,0 ni, 99,1 id, 0,0 wa, 0,0 hi, 0,0 si, 0,0 st"; + let pct = parse_cpu_top(out).expect("parse comma-locale cpu"); + assert!((pct - 0.9).abs() < 0.2, "expected ~0.9, got {pct}"); +} + +#[test] +fn cpu_comma_decimal_reported_values() { + for (idle, expected) in [("99,9", 0.1), ("99,8", 0.2)] { + let out = format!("%Cpu(s): 0,1 us, 0,0 sy, 0,0 ni, {idle} id, 0,0 wa"); + let pct = parse_cpu_top(&out).expect("parse comma-locale cpu"); + assert!( + (pct - expected).abs() < 0.2, + "expected ~{expected}, got {pct}" + ); + } +} + +#[test] +fn cpu_comma_decimal_fully_idle() { + let out = "%Cpu(s): 0,0 us, 0,0 sy, 0,0 ni,100,0 id, 0,0 wa, 0,0 hi, 0,0 si, 0,0 st"; + let pct = parse_cpu_top(out).expect("parse comma-locale cpu"); + assert!(pct.abs() < 0.2, "expected ~0.0, got {pct}"); +} + +#[test] +fn cpu_comma_decimal_old_format() { + let out = "Cpu(s): 2,3%us, 0,7%sy, 0,0%ni, 96,7%id, 0,3%wa, 0,0%hi, 0,0%si, 0,0%st"; + let pct = parse_cpu_top(out).expect("parse comma-locale centos7 cpu"); + assert!((pct - 3.3).abs() < 0.2, "expected ~3.3, got {pct}"); +} + +#[test] +fn cpu_macos_comma_decimal_locale() { + let out = "CPU usage: 3,17% user, 1,56% sys, 95,26% idle"; + let pct = parse_cpu_top_macos(out).expect("parse comma-locale macos cpu"); + assert!((pct - 4.74).abs() < 0.2, "expected ~4.74, got {pct}"); +} + #[test] fn cpu_macos_top() { let out = "CPU usage: 3.17% user, 1.56% sys, 95.26% idle"; From b98f755b60619b5b1352bbf809a5248eac71f5ca Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 21:58:36 +0400 Subject: [PATCH 04/13] fix(config): resolve ssh_config Include the way OpenSSH does A relative pattern such as "Include conf.d/*.conf" was looked up in whatever directory the app was launched from - "/" for a GUI started from Finder - so every host in a conf.d split silently went missing. ssh_config(5) resolves those against ~/.ssh. Anchor the pattern to the config's own directory, and fix what the same code path got wrong alongside it: full glob(7) instead of a single '*' in the file name, several pathnames on one Include line, quoted paths, and an Include inside a Host block no longer discarding the rest of that block. An Include that matches nothing is now logged rather than dropped in silence. --- Cargo.lock | 2 + crates/omnyssh-core/Cargo.toml | 4 + crates/omnyssh-core/src/config/ssh_config.rs | 170 ++++++++----- .../omnyssh-core/tests/ssh_config_parser.rs | 239 +++++++++++++++++- 4 files changed, 346 insertions(+), 69 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f2724fd..b85aa6d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3198,6 +3198,7 @@ dependencies = [ "chrono", "dirs 5.0.1", "flate2", + "glob", "reqwest 0.12.28", "russh", "russh-keys", @@ -3207,6 +3208,7 @@ dependencies = [ "serde", "sha2", "tar", + "tempfile", "thiserror 2.0.18", "tokio", "toml 0.8.23", diff --git a/crates/omnyssh-core/Cargo.toml b/crates/omnyssh-core/Cargo.toml index b01bfed..00ad3dc 100644 --- a/crates/omnyssh-core/Cargo.toml +++ b/crates/omnyssh-core/Cargo.toml @@ -36,6 +36,9 @@ chrono = { workspace = true } # Cross-platform paths (P-CROSS-01) dirs = { workspace = true } +# glob(7) patterns in ssh_config `Include` directives +glob = "0.3" + # Terminal emulator — vt100 screen model (PTY is provided by russh channels) vt100 = { package = "vt100-omnyssh", version = "0.15" } @@ -49,3 +52,4 @@ self-replace = "1" [dev-dependencies] tokio = { workspace = true, features = ["full", "test-util"] } +tempfile = "3" diff --git a/crates/omnyssh-core/src/config/ssh_config.rs b/crates/omnyssh-core/src/config/ssh_config.rs index cc34538..0de080b 100644 --- a/crates/omnyssh-core/src/config/ssh_config.rs +++ b/crates/omnyssh-core/src/config/ssh_config.rs @@ -13,10 +13,12 @@ use crate::ssh::client::{Host, HostSource}; /// Parses the text of an SSH config file and returns all non-wildcard hosts. /// /// `Host *` entries are silently skipped. -/// `Include` recursion is limited to 3 levels to prevent cycles. +/// `Include` recursion is limited to 3 levels to prevent cycles; relative +/// patterns resolve against `~/.ssh`, as `ssh_config(5)` specifies for a user +/// configuration. pub fn parse_ssh_config(content: &str) -> Vec { let mut visited: HashSet = HashSet::new(); - parse_content(content, 0, &mut visited) + parse_content(content, &default_include_base(), 0, &mut visited) } /// Loads and parses an SSH config file from disk. @@ -26,20 +28,39 @@ pub fn parse_ssh_config(content: &str) -> Vec { pub fn load_from_file(path: &Path) -> anyhow::Result> { let content = std::fs::read_to_string(path) .map_err(|e| anyhow::anyhow!("Failed to read {}: {}", path.display(), e))?; - Ok(parse_ssh_config(&content)) + let base = path + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .map_or_else(default_include_base, Path::to_path_buf); + let mut visited: HashSet = HashSet::new(); + Ok(parse_content(&content, &base, 0, &mut visited)) +} + +/// `~/.ssh` — where `ssh_config(5)` resolves a relative `Include` in a user +/// configuration. Never the process working directory. +fn default_include_base() -> PathBuf { + dirs::home_dir().map(|h| h.join(".ssh")).unwrap_or_default() } // --------------------------------------------------------------------------- // Internal helpers // --------------------------------------------------------------------------- -fn parse_content(content: &str, depth: usize, visited: &mut HashSet) -> Vec { +fn parse_content( + content: &str, + base: &Path, + depth: usize, + visited: &mut HashSet, +) -> Vec { if depth > 3 { return Vec::new(); } let mut hosts: Vec = Vec::new(); let mut current: Option = None; + // Hosts pulled in by an Include that sat inside a Host block. Appended once + // the enclosing host is flushed, so the list keeps the config's own order. + let mut deferred: Vec = Vec::new(); // True when we are inside a wildcard `Host *` block (skip directives). let mut in_wildcard = false; @@ -59,6 +80,7 @@ fn parse_content(content: &str, depth: usize, visited: &mut HashSet) -> if let Some(h) = current.take() { hosts.push(h); } + hosts.append(&mut deferred); in_wildcard = value.contains('*') || value.contains('?'); if !in_wildcard { let h = Host { @@ -98,30 +120,38 @@ fn parse_content(content: &str, depth: usize, visited: &mut HashSet) -> h.proxy_jump = Some(value.to_string()); } } + // An Include may sit inside a Host block; the enclosing host keeps + // collecting directives after it. "include" => { - // Flush the current host before processing Include. - if let Some(h) = current.take() { - hosts.push(h); - } - in_wildcard = false; - let expanded = expand_tilde(value); - for path in expand_include_glob(&expanded) { - // Canonicalise to catch cycles (symlinks, etc.). - let canonical = path.canonicalize().unwrap_or_else(|e| { - tracing::warn!( - path = %path.display(), - error = %e, - "canonicalize failed; symlink-cycle detection disabled for this path" - ); - path.clone() - }); - if !visited.insert(canonical) { - continue; // already visited — break cycle + let sink = if current.is_some() { + &mut deferred + } else { + &mut hosts + }; + for pattern in split_include_patterns(value) { + let resolved = resolve_include(&pattern, base); + let matched = expand_include_glob(&resolved); + if matched.is_empty() { + tracing::warn!(pattern = %resolved.display(), "Include matched no files"); } - match std::fs::read_to_string(&path) { - Ok(sub) => hosts.extend(parse_content(&sub, depth + 1, visited)), - Err(e) => { - tracing::warn!(path = %path.display(), error = %e, "Include file unreadable") + for path in matched { + // Canonicalise to catch cycles (symlinks, etc.). + let canonical = path.canonicalize().unwrap_or_else(|e| { + tracing::warn!( + path = %path.display(), + error = %e, + "canonicalize failed; symlink-cycle detection disabled for this path" + ); + path.clone() + }); + if !visited.insert(canonical) { + continue; // already visited — break cycle + } + match std::fs::read_to_string(&path) { + Ok(sub) => sink.extend(parse_content(&sub, base, depth + 1, visited)), + Err(e) => { + tracing::warn!(path = %path.display(), error = %e, "Include file unreadable") + } } } } @@ -130,10 +160,11 @@ fn parse_content(content: &str, depth: usize, visited: &mut HashSet) -> } } - // Flush the last pending host. + // Flush the last pending host, then anything its Include pulled in. if let Some(h) = current.take() { hosts.push(h); } + hosts.append(&mut deferred); // Fallback: if HostName was never set, use the alias as the address. for h in &mut hosts { @@ -181,48 +212,59 @@ fn expand_tilde(s: &str) -> String { s.to_string() } -/// Resolves an Include pattern to a list of file paths. +/// Splits an `Include` value into its pathnames. /// -/// Supports a single `*` wildcard in the file-name component only. -/// The parent directory must exist; `*` in path segments other than the -/// last one is not supported (matches OpenSSH behaviour). -fn expand_include_glob(pattern: &str) -> Vec { - let path = PathBuf::from(pattern); - let parent = match path.parent() { - Some(p) if !p.as_os_str().is_empty() => p.to_path_buf(), - _ => PathBuf::from("."), - }; - let file_name = match path.file_name().and_then(|f| f.to_str()) { - Some(n) => n.to_string(), - None => return Vec::new(), - }; +/// `ssh_config(5)` allows several pathnames on one line, and a path containing +/// spaces may be double-quoted. +fn split_include_patterns(value: &str) -> Vec { + let mut patterns = Vec::new(); + let mut current = String::new(); + let mut quoted = false; + + for c in value.chars() { + match c { + '"' => quoted = !quoted, + c if c.is_whitespace() && !quoted => { + if !current.is_empty() { + patterns.push(std::mem::take(&mut current)); + } + } + c => current.push(c), + } + } + if !current.is_empty() { + patterns.push(current); + } + patterns +} - if !file_name.contains('*') { - return if path.is_file() { - vec![path] - } else { - Vec::new() - }; +/// Anchors an `Include` pattern: absolute and `~/` patterns stand alone, a +/// relative one resolves against `base` — never the process working directory. +fn resolve_include(pattern: &str, base: &Path) -> PathBuf { + let expanded = expand_tilde(pattern); + let path = PathBuf::from(&expanded); + if path.is_absolute() { + path + } else { + base.join(path) } +} - // Simple single-`*` glob: match prefix and suffix. - let (prefix, suffix) = file_name.split_once('*').unwrap_or((&file_name, "")); - match std::fs::read_dir(&parent) { - Ok(entries) => { - let mut paths: Vec = entries - .flatten() - .filter(|e| { - let name = e.file_name(); - let name = name.to_string_lossy(); - name.starts_with(prefix) && name.ends_with(suffix) - }) - .map(|e| e.path()) - .filter(|p| p.is_file()) - .collect(); - paths.sort(); // deterministic order - paths +/// Resolves an Include pattern to the files it matches, in a stable order. +/// +/// Full glob(7) syntax, as `ssh_config(5)` specifies. Directories that match +/// are skipped. +fn expand_include_glob(pattern: &Path) -> Vec { + let Some(pattern) = pattern.to_str() else { + return Vec::new(); + }; + match glob::glob(pattern) { + // `glob` yields matches in sorted order, so the host list is stable. + Ok(paths) => paths.flatten().filter(|p| p.is_file()).collect(), + Err(e) => { + tracing::warn!(pattern, error = %e, "invalid Include pattern"); + Vec::new() } - Err(_) => Vec::new(), } } diff --git a/crates/omnyssh-core/tests/ssh_config_parser.rs b/crates/omnyssh-core/tests/ssh_config_parser.rs index 9fd2d7e..09bf452 100644 --- a/crates/omnyssh-core/tests/ssh_config_parser.rs +++ b/crates/omnyssh-core/tests/ssh_config_parser.rs @@ -1,5 +1,234 @@ -// Integration tests for the SSH config parser are located alongside the -// implementation in `src/config/ssh_config.rs` (the `#[cfg(test)]` block). -// -// This file is intentionally minimal: the parser's unit tests live in the -// module itself and cover the parsing logic directly. +//! Integration tests for `Include` handling in the SSH config parser. +//! +//! The parsing rules themselves are covered by the unit tests next to the +//! implementation. These need a real directory tree on disk, so they live here. + +use std::fs; +use std::path::Path; + +use omnyssh_core::config::ssh_config::load_from_file; + +/// The layout from the field report: a top-level config plus a `conf.d` split. +fn write_fixture(root: &Path) -> std::path::PathBuf { + let ssh = root.join(".ssh"); + let conf_d = ssh.join("conf.d"); + fs::create_dir_all(&conf_d).expect("create fixture tree"); + + fs::write( + conf_d.join("10-vps.conf"), + "Host vps\n HostName 1.2.3.4\n User root\n", + ) + .expect("write 10-vps.conf"); + fs::write( + conf_d.join("20-work-test.conf"), + "Host work-test\n HostName 10.0.0.2\n", + ) + .expect("write 20-work-test.conf"); + fs::write( + conf_d.join("30-work-stage.conf"), + "Host work-stage\n HostName 10.0.0.3\n", + ) + .expect("write 30-work-stage.conf"); + + ssh.join("config") +} + +/// Writes `body`, then the direct host every case shares, and parses the result. +/// The fixture tree must already exist. +fn hosts_for(root: &Path, body: &str) -> Vec { + let config = root.join(".ssh").join("config"); + fs::write( + &config, + format!("{body}\n\nHost local-direct\n HostName 127.0.0.1\n"), + ) + .expect("write config"); + + load_from_file(&config) + .expect("parse config") + .into_iter() + .map(|h| h.name) + .collect() +} + +#[test] +fn relative_include_resolves_against_the_config_directory() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let names = hosts_for(tmp.path(), "Include conf.d/*.conf"); + + assert_eq!(names, ["vps", "work-test", "work-stage", "local-direct"]); +} + +/// The defect this file exists for: a relative `Include` used to be read from +/// the directory the app happened to be launched in, so a GUI started from +/// Finder (working directory `/`) found none of the included hosts. +#[test] +fn a_relative_include_ignores_the_process_working_directory() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + + // A decoy tree that a working-directory lookup would find instead. + let decoy = tempfile::tempdir().expect("tempdir"); + fs::create_dir_all(decoy.path().join("conf.d")).expect("create decoy tree"); + fs::write( + decoy.path().join("conf.d").join("99-decoy.conf"), + "Host decoy + HostName 6.6.6.6 +", + ) + .expect("write decoy"); + + let previous = std::env::current_dir().expect("read working directory"); + std::env::set_current_dir(decoy.path()).expect("enter decoy directory"); + let names = hosts_for(tmp.path(), "Include conf.d/*.conf"); + std::env::set_current_dir(previous).expect("restore working directory"); + + assert_eq!(names, ["vps", "work-test", "work-stage", "local-direct"]); +} + +#[test] +fn absolute_and_glob7_include_patterns_resolve() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let conf_d = tmp.path().join(".ssh").join("conf.d"); + + let cases = [ + format!("Include {}/*.conf", conf_d.display()), + "Include conf.d/?0-*.conf".to_string(), + "Include conf.d/[123]0-*.conf".to_string(), + "Include con*/*.conf".to_string(), + ]; + for body in cases { + let names = hosts_for(tmp.path(), &body); + assert_eq!( + names, + ["vps", "work-test", "work-stage", "local-direct"], + "pattern did not match: {body}" + ); + } +} + +#[test] +fn a_second_wildcard_in_one_filename_matches() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let names = hosts_for(tmp.path(), "Include conf.d/*-work-*.conf"); + + assert_eq!(names, ["work-test", "work-stage", "local-direct"]); +} + +#[test] +fn several_pathnames_on_one_include_line_are_all_read() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let names = hosts_for( + tmp.path(), + "Include conf.d/10-vps.conf conf.d/20-work-test.conf", + ); + + assert_eq!(names, ["vps", "work-test", "local-direct"]); +} + +#[test] +fn a_quoted_pathname_keeps_its_spaces() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + fs::write( + tmp.path() + .join(".ssh") + .join("conf.d") + .join("with space.conf"), + "Host spaced\n HostName 10.0.0.4\n", + ) + .expect("write fixture"); + + let names = hosts_for(tmp.path(), "Include \"conf.d/with space.conf\""); + + assert_eq!(names, ["spaced", "local-direct"]); +} + +#[test] +fn an_include_inside_a_host_block_keeps_the_enclosing_host() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let config = tmp.path().join(".ssh").join("config"); + fs::write( + &config, + "Host gate\n Include conf.d/10-vps.conf\n HostName 10.0.0.9\n User ops\n", + ) + .expect("write config"); + + let hosts = load_from_file(&config).expect("parse config"); + let gate = hosts + .iter() + .find(|h| h.name == "gate") + .expect("enclosing host survives the Include"); + + assert_eq!(gate.hostname, "10.0.0.9"); + assert_eq!(gate.user, "ops"); + assert!(hosts.iter().any(|h| h.name == "vps")); +} + +#[test] +fn an_include_that_matches_nothing_is_a_no_op() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + + for body in ["Include conf.d/nope*.conf", "Include conf.d/absent.conf"] { + assert_eq!(hosts_for(tmp.path(), body), ["local-direct"]); + } +} + +#[test] +fn a_directory_matching_the_pattern_is_skipped() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + fs::create_dir_all(tmp.path().join(".ssh").join("conf.d").join("40-dir.conf")) + .expect("create decoy directory"); + + let names = hosts_for(tmp.path(), "Include conf.d/*.conf"); + + assert_eq!(names, ["vps", "work-test", "work-stage", "local-direct"]); +} + +#[test] +fn a_nested_relative_include_resolves_against_the_same_base() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let conf_d = tmp.path().join(".ssh").join("conf.d"); + fs::write( + conf_d.join("10-vps.conf"), + "Host vps\n HostName 1.2.3.4\nInclude conf.d/40-deep.conf\n", + ) + .expect("write 10-vps.conf"); + fs::write( + conf_d.join("40-deep.conf"), + "Host deep\n HostName 10.0.0.5\n", + ) + .expect("write 40-deep.conf"); + + let names = hosts_for(tmp.path(), "Include conf.d/10-vps.conf"); + + assert_eq!(names, ["vps", "deep", "local-direct"]); +} + +#[test] +fn an_include_cycle_terminates() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let conf_d = tmp.path().join(".ssh").join("conf.d"); + fs::write( + conf_d.join("a.conf"), + "Host a\n HostName 10.0.0.6\nInclude conf.d/b.conf\n", + ) + .expect("write a.conf"); + fs::write( + conf_d.join("b.conf"), + "Host b\n HostName 10.0.0.7\nInclude conf.d/a.conf\n", + ) + .expect("write b.conf"); + + let names = hosts_for(tmp.path(), "Include conf.d/a.conf"); + + assert_eq!(names, ["a", "b", "local-direct"]); +} From 6e7b0f04654370a663b239e89cca6aa21d195772 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 21:59:00 +0400 Subject: [PATCH 05/13] docs(changelog): note the Include, locale and reconnect fixes --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7956099..fc476f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ Versions follow [Semantic Versioning](https://semver.org/). ## Unreleased ### Bug Fixes +- **Hosts split across `Include` files are imported.** A relative pattern — `Include conf.d/*.conf`, the form nearly every split-config guide prints — was looked for in whatever directory the app happened to be launched from, which for the desktop app started from Finder or the application menu is `/`. Nothing matched, so every host defined in `~/.ssh/conf.d` was missing and had to be added by hand. Those patterns now resolve against `~/.ssh`, the way `ssh` itself resolves them. Four things in the same code path were fixed alongside it: full glob patterns work (`?`, `[abc]`, a wildcard in a directory name, and more than one `*` in a file name), several pathnames on one `Include` line are all read instead of none, a quoted path keeps its spaces, and an `Include` written inside a `Host` block no longer swallows that host's remaining settings. An `Include` that matches nothing is written to the log rather than passing in silence. +- **CPU no longer reads ~91–100 % on an idle server whose system language isn't English.** Monitoring reads the idle percentage from `top` and shows the rest as used. On a server set to a language with a comma decimal separator, `top` prints `99,1 id`, and the parser split that line on commas — reading the idle value as `1` and reporting 99 % used on a machine doing nothing. The same servers could show RAM and Disk as N/A, because `free` and `df` translate the labels being looked for, and the process list could come back empty. The monitoring commands now run in a fixed locale, and the CPU parsers understand a decimal comma on their own for hosts where that cannot be set. Process names keep the server's own character set. +- **A host that answers SSH but not shell commands is no longer re-logged-in every 30 seconds.** Network appliances — firewalls, switches — authenticate fine but cannot run `top` or `free`, and a round of failed metric commands was treated as a dead connection. The retry delay was reset on every successful login, so it never grew past its first step: one login every 30 seconds, indefinitely, and as often as every 10 seconds in the desktop app, whose refresh timer also cut the retry delay short. The delay now escalates properly and is left alone by the refresh timer. Separately, a device that ignores SSH keepalives was being disconnected after 30 seconds even while its commands still worked; connections are now held open, with liveness still bounded by the keepalive limit. - **`ProxyJump` hosts now connect through their bastion.** A host with `ProxyJump` in `~/.ssh/config` was parsed but never routed: the terminal refused to open it, while metrics, SFTP, snippets and key setup quietly dialled the target address direct — which for an internal host meant every connection failed or, worse, landed somewhere else on that address. Every native SSH path now walks the jump chain the way `ssh -J` does, connecting and authenticating each bastion in turn and tunnelling the next hop over it. - The jump alias is resolved against your host list, so `ProxyJump public-proxy` picks up that entry's `HostName`, `User`, `Port` and `IdentityFile`. An alias that matches no entry is used as a literal hostname. - Multi-hop values (`ProxyJump first,second`), inline `user@host:port` overrides, IPv6 literals, bastions that are themselves behind a bastion, and the `ProxyJump none` opt-out all behave as OpenSSH does — including its precedence rule that only the first hop of a list contributes bastions of its own. Chains that loop, or run past ten hops, are reported instead of hanging. From 78b52080369a97d8fbde12a1448ab12fd83251a4 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 22:24:50 +0400 Subject: [PATCH 06/13] fix(poll): stop the backoff wait spinning on a closed refresh channel Once every refresh sender is dropped, recv() returns Ready(None) immediately and forever, so the select loop re-armed without ever yielding - one core pinned for the whole delay. Two hosts sharing a name reach it: refresh_txs is keyed by name, so the first poller's sender is dropped while its task still runs. The new poller test also covers what the backoff change itself lacked: it fails if a refresh signal is allowed to cut the wait short. --- crates/omnyssh-core/src/ssh/pool.rs | 30 +++++-- crates/omnyssh-core/tests/poll_backoff.rs | 103 ++++++++++++++++++++++ 2 files changed, 126 insertions(+), 7 deletions(-) create mode 100644 crates/omnyssh-core/tests/poll_backoff.rs diff --git a/crates/omnyssh-core/src/ssh/pool.rs b/crates/omnyssh-core/src/ssh/pool.rs index e816a74..a86a9df 100644 --- a/crates/omnyssh-core/src/ssh/pool.rs +++ b/crates/omnyssh-core/src/ssh/pool.rs @@ -238,15 +238,20 @@ async fn wait_or_refresh(delay: Duration, refresh_rx: &mut mpsc::Receiver<()>) { /// `refresh_all` on its own timer, which is indistinguishable from a keypress /// here and would otherwise retry a failing host every few seconds. async fn wait_backoff(delay: Duration, refresh_rx: &mut mpsc::Receiver<()>) { - let deadline = Instant::now() + delay; + let sleep = tokio::time::sleep(delay); + tokio::pin!(sleep); loop { - let left = deadline.saturating_duration_since(Instant::now()); - if left.is_zero() { - return; - } tokio::select! { - _ = tokio::time::sleep(left) => return, - _ = refresh_rx.recv() => {} + () = &mut sleep => return, + signal = refresh_rx.recv() => { + // `None` means every sender is gone and `recv` will return it + // immediately from now on — stop selecting on it, or the task + // spins without ever yielding. + if signal.is_none() { + sleep.await; + return; + } + } } } } @@ -456,6 +461,17 @@ mod tests { assert_eq!(start.elapsed(), Duration::from_secs(300)); } + #[tokio::test(start_paused = true)] + async fn wait_backoff_serves_out_its_delay_once_every_sender_is_gone() { + let (tx, mut rx) = mpsc::channel::<()>(4); + drop(tx); + + let start = tokio::time::Instant::now(); + wait_backoff(Duration::from_secs(300), &mut rx).await; + + assert_eq!(start.elapsed(), Duration::from_secs(300)); + } + #[tokio::test(start_paused = true)] async fn wait_or_refresh_still_returns_early() { let (tx, mut rx) = mpsc::channel::<()>(4); diff --git a/crates/omnyssh-core/tests/poll_backoff.rs b/crates/omnyssh-core/tests/poll_backoff.rs new file mode 100644 index 0000000..b963759 --- /dev/null +++ b/crates/omnyssh-core/tests/poll_backoff.rs @@ -0,0 +1,103 @@ +//! The reconnect schedule of the metrics poller, driven through `PollManager`. +//! +//! The listener accepts a TCP connection and drops it, so every SSH connect +//! fails the way an unreachable host does — enough to exercise the backoff +//! without an SSH server. + +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Arc; +use std::time::Duration; + +use tokio::net::TcpListener; +use tokio::sync::mpsc; + +use omnyssh_core::event::CoreEvent; +use omnyssh_core::ssh::client::Host; +use omnyssh_core::ssh::pool::PollManager; + +/// Counts connections, answering none of them. +async fn dead_listener() -> (u16, Arc) { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let port = listener.local_addr().expect("local addr").port(); + let dials = Arc::new(AtomicUsize::new(0)); + + let counter = Arc::clone(&dials); + tokio::spawn(async move { + while let Ok((stream, _)) = listener.accept().await { + counter.fetch_add(1, Ordering::SeqCst); + drop(stream); + } + }); + + (port, dials) +} + +fn unreachable_host(port: u16) -> Host { + Host { + name: String::from("probe"), + hostname: String::from("127.0.0.1"), + port, + ..Host::default() + } +} + +/// Drains events so a full channel can never stall the poller under test. +fn drain(mut rx: mpsc::Receiver) { + tokio::spawn(async move { while rx.recv().await.is_some() {} }); +} + +/// Waits until `dials` reaches `target`, or `limit` of the test clock elapses. +/// Sampling rather than sleeping a fixed span keeps the count off the critical +/// path: the listener increments it from its own task. +async fn wait_for_dials(dials: &AtomicUsize, target: usize, limit: Duration) -> bool { + tokio::time::timeout(limit, async { + while dials.load(Ordering::SeqCst) < target { + tokio::time::sleep(Duration::from_millis(100)).await; + } + }) + .await + .is_ok() +} + +#[tokio::test(start_paused = true)] +async fn a_refresh_signal_does_not_shorten_the_reconnect_backoff() { + let (port, dials) = dead_listener().await; + let (tx, rx) = mpsc::channel(64); + drain(rx); + + let manager = PollManager::start(vec![unreachable_host(port)], tx, Duration::from_secs(30)); + assert!( + wait_for_dials(&dials, 1, Duration::from_secs(60)).await, + "the poller never dialled at all" + ); + + // Twelve nudges over two minutes — the shape of the GUI's refresh timer at + // its shortest setting. + for _ in 0..12 { + manager.refresh_all(); + tokio::time::sleep(Duration::from_secs(10)).await; + } + manager.shutdown(); + + // Backoff runs 30, 60, 120: four dials at most in the 120 s that follow the + // first. Honouring the signals instead dials on every one of them. + let dialled = dials.load(Ordering::SeqCst); + assert!( + dialled <= 4, + "expected the backoff schedule to hold, got {dialled} dials" + ); +} + +#[tokio::test(start_paused = true)] +async fn an_unreachable_host_keeps_retrying_on_its_own_schedule() { + let (port, dials) = dead_listener().await; + let (tx, rx) = mpsc::channel(64); + drain(rx); + + let manager = PollManager::start(vec![unreachable_host(port)], tx, Duration::from_secs(30)); + let retried = wait_for_dials(&dials, 2, Duration::from_secs(300)).await; + manager.shutdown(); + + // A host that stops answering is retried, not abandoned. + assert!(retried, "the poller stopped retrying an unreachable host"); +} From 24a1c56d17c24c8b59892f200867d6b15d416df7 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 22:25:01 +0400 Subject: [PATCH 07/13] fix(metrics): clear LC_ALL so the numeric pin reaches ps LC_ALL outranks every other LC_* category, so setting LC_NUMERIC alone did nothing on a host that exports LC_ALL - the very hosts the pin is for. Clear it instead, which still leaves LC_CTYPE to the host so process names keep their own character set, and normalise a decimal comma in the ps columns for hosts the pin cannot reach. --- crates/omnyssh-core/src/ssh/metrics.rs | 7 ++++++- crates/omnyssh-core/src/ssh/pool.rs | 16 +++++++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/metrics.rs b/crates/omnyssh-core/src/ssh/metrics.rs index f6a083d..03d2fc0 100644 --- a/crates/omnyssh-core/src/ssh/metrics.rs +++ b/crates/omnyssh-core/src/ssh/metrics.rs @@ -379,7 +379,12 @@ pub fn parse_top_processes(output: &str) -> Option> { continue; }; // The first two columns must be numeric — this skips the header row. - let (Ok(cpu), Ok(mem)) = (cpu_str.parse::(), mem_str.parse::()) else { + // `ps` prints them in the server's locale, so a decimal comma is normalised + // here too; a non-numeric token still fails to parse. + let (Ok(cpu), Ok(mem)) = ( + normalize_decimal_commas(cpu_str).parse::(), + normalize_decimal_commas(mem_str).parse::(), + ) else { continue; }; let name = fields.collect::>().join(" "); diff --git a/crates/omnyssh-core/src/ssh/pool.rs b/crates/omnyssh-core/src/ssh/pool.rs index a86a9df..fdb650c 100644 --- a/crates/omnyssh-core/src/ssh/pool.rs +++ b/crates/omnyssh-core/src/ssh/pool.rs @@ -45,7 +45,7 @@ const UPTIME_CMD: &str = "env LC_ALL=C uptime 2>/dev/null"; const CPU_MACOS_CMD: &str = "env LC_ALL=C top -l 1 -n 0 2>/dev/null | grep 'CPU usage'"; // `ps` output reaches the user, so keep the host's LC_CTYPE: under a full // `LC_ALL=C` GNU ps replaces every non-ASCII byte of a process name with '?'. -const PS_LOCALE: &str = "env LC_NUMERIC=C LC_MESSAGES=C"; +const PS_LOCALE: &str = "env LC_ALL= LC_NUMERIC=C LC_MESSAGES=C"; struct BackoffState { step: usize, @@ -498,16 +498,22 @@ mod tests { // Both `ps` invocations are pinned, so a comma-decimal host still parses. assert_eq!(cmd.matches(PS_LOCALE).count(), 2); - // LC_CTYPE stays with the host: process names reach the user verbatim. - assert!(!cmd.contains("LC_ALL")); + // LC_ALL is cleared rather than set: it outranks LC_NUMERIC, so leaving a + // host's own LC_ALL in place would defeat the pin. LC_CTYPE still falls + // through to the host, so process names reach the user verbatim. + assert!(cmd.contains("LC_ALL=")); + assert!(!cmd.contains("LC_ALL=C")); + assert!(!cmd.contains("LC_CTYPE")); } #[test] fn top_processes_command_excludes_monitor_pid_chain() { let cmd = top_processes_command("-eo pid=,ppid=,pcpu=,pmem=,comm= --sort=-pcpu"); - // The grandparent PID is resolved before the pipeline runs. - assert!(cmd.contains("ps -o ppid= -p $PPID")); + // The grandparent PID is resolved in a substitution that runs before + // the pipeline does. + assert!(cmd.starts_with("g=$(")); + assert!(cmd.contains("ps -o ppid= -p $PPID 2>/dev/null | tr -d ' ')")); // The awk filter binds the shell, its parent sshd and the grandparent. assert!(cmd.contains("-v s=$$")); assert!(cmd.contains("-v p=$PPID")); From ff2f7ad392edd2de34a8b474737a20d0b8cc3017 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 22:25:01 +0400 Subject: [PATCH 08/13] fix(config): keep an awkward home directory from breaking Include The base directory is a real path, not a pattern, so a home containing a glob metacharacter turned every relative Include into a pattern that matched nothing. Escape it before joining. A relative Include with no home directory to anchor it is now dropped with a warning rather than silently resolved against the process working directory, which is the defect this parser was fixed for. --- crates/omnyssh-core/src/config/ssh_config.rs | 41 +++++++++++-------- .../omnyssh-core/tests/ssh_config_parser.rs | 26 +++++++++++- 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/crates/omnyssh-core/src/config/ssh_config.rs b/crates/omnyssh-core/src/config/ssh_config.rs index 0de080b..1d73dab 100644 --- a/crates/omnyssh-core/src/config/ssh_config.rs +++ b/crates/omnyssh-core/src/config/ssh_config.rs @@ -18,7 +18,7 @@ use crate::ssh::client::{Host, HostSource}; /// configuration. pub fn parse_ssh_config(content: &str) -> Vec { let mut visited: HashSet = HashSet::new(); - parse_content(content, &default_include_base(), 0, &mut visited) + parse_content(content, default_include_base().as_deref(), 0, &mut visited) } /// Loads and parses an SSH config file from disk. @@ -31,15 +31,15 @@ pub fn load_from_file(path: &Path) -> anyhow::Result> { let base = path .parent() .filter(|p| !p.as_os_str().is_empty()) - .map_or_else(default_include_base, Path::to_path_buf); + .map_or_else(default_include_base, |p| Some(p.to_path_buf())); let mut visited: HashSet = HashSet::new(); - Ok(parse_content(&content, &base, 0, &mut visited)) + Ok(parse_content(&content, base.as_deref(), 0, &mut visited)) } /// `~/.ssh` — where `ssh_config(5)` resolves a relative `Include` in a user /// configuration. Never the process working directory. -fn default_include_base() -> PathBuf { - dirs::home_dir().map(|h| h.join(".ssh")).unwrap_or_default() +fn default_include_base() -> Option { + dirs::home_dir().map(|h| h.join(".ssh")) } // --------------------------------------------------------------------------- @@ -48,7 +48,7 @@ fn default_include_base() -> PathBuf { fn parse_content( content: &str, - base: &Path, + base: Option<&Path>, depth: usize, visited: &mut HashSet, ) -> Vec { @@ -129,10 +129,13 @@ fn parse_content( &mut hosts }; for pattern in split_include_patterns(value) { - let resolved = resolve_include(&pattern, base); + let Some(resolved) = resolve_include(&pattern, base) else { + tracing::warn!(pattern, "relative Include with no home directory"); + continue; + }; let matched = expand_include_glob(&resolved); if matched.is_empty() { - tracing::warn!(pattern = %resolved.display(), "Include matched no files"); + tracing::warn!(pattern = resolved, "Include matched no files"); } for path in matched { // Canonicalise to catch cycles (symlinks, etc.). @@ -240,24 +243,26 @@ fn split_include_patterns(value: &str) -> Vec { /// Anchors an `Include` pattern: absolute and `~/` patterns stand alone, a /// relative one resolves against `base` — never the process working directory. -fn resolve_include(pattern: &str, base: &Path) -> PathBuf { +/// +/// `None` when the pattern is relative and there is no base to anchor it to; +/// dropping the include is the only honest option, since resolving it would +/// silently read the process working directory. +fn resolve_include(pattern: &str, base: Option<&Path>) -> Option { let expanded = expand_tilde(pattern); - let path = PathBuf::from(&expanded); - if path.is_absolute() { - path - } else { - base.join(path) + if Path::new(&expanded).is_absolute() { + return Some(expanded); } + // The base is a real path, not a pattern: escape it so a home directory + // containing `[` or `*` cannot swallow the include. + let base = glob::Pattern::escape(base?.to_str()?); + Some(format!("{}/{}", base.trim_end_matches('/'), expanded)) } /// Resolves an Include pattern to the files it matches, in a stable order. /// /// Full glob(7) syntax, as `ssh_config(5)` specifies. Directories that match /// are skipped. -fn expand_include_glob(pattern: &Path) -> Vec { - let Some(pattern) = pattern.to_str() else { - return Vec::new(); - }; +fn expand_include_glob(pattern: &str) -> Vec { match glob::glob(pattern) { // `glob` yields matches in sorted order, so the host list is stable. Ok(paths) => paths.flatten().filter(|p| p.is_file()).collect(), diff --git a/crates/omnyssh-core/tests/ssh_config_parser.rs b/crates/omnyssh-core/tests/ssh_config_parser.rs index 09bf452..63a8993 100644 --- a/crates/omnyssh-core/tests/ssh_config_parser.rs +++ b/crates/omnyssh-core/tests/ssh_config_parser.rs @@ -78,10 +78,18 @@ fn a_relative_include_ignores_the_process_working_directory() { ) .expect("write decoy"); - let previous = std::env::current_dir().expect("read working directory"); + // Restore on the way out even if the parse panics: the working directory is + // process-global and the decoy is deleted when this test ends. + struct RestoreCwd(std::path::PathBuf); + impl Drop for RestoreCwd { + fn drop(&mut self) { + let _ = std::env::set_current_dir(&self.0); + } + } + let guard = RestoreCwd(std::env::current_dir().expect("read working directory")); std::env::set_current_dir(decoy.path()).expect("enter decoy directory"); let names = hosts_for(tmp.path(), "Include conf.d/*.conf"); - std::env::set_current_dir(previous).expect("restore working directory"); + drop(guard); assert_eq!(names, ["vps", "work-test", "work-stage", "local-direct"]); } @@ -108,6 +116,20 @@ fn absolute_and_glob7_include_patterns_resolve() { } } +/// A home directory containing a glob metacharacter must not be treated as a +/// pattern: the base is a real path, only the `Include` value is a pattern. +#[test] +fn a_base_directory_containing_glob_metacharacters_still_resolves() { + let tmp = tempfile::tempdir().expect("tempdir"); + let awkward = tmp.path().join("us[er]"); + fs::create_dir_all(&awkward).expect("create awkward base"); + write_fixture(&awkward); + + let names = hosts_for(&awkward, "Include conf.d/*.conf"); + + assert_eq!(names, ["vps", "work-test", "work-stage", "local-direct"]); +} + #[test] fn a_second_wildcard_in_one_filename_matches() { let tmp = tempfile::tempdir().expect("tempdir"); From 9ec09d21455bb29005a38eb635fc172845e80aa4 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 23:30:30 +0400 Subject: [PATCH 09/13] fix(poll): stop the poll wait spinning on a closed refresh channel Same hole as the backoff wait: once every sender is dropped, recv() completes immediately and forever, so returning on it made the poll loop spin. Serve out the delay instead. --- crates/omnyssh-core/src/ssh/pool.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/pool.rs b/crates/omnyssh-core/src/ssh/pool.rs index fdb650c..4273c45 100644 --- a/crates/omnyssh-core/src/ssh/pool.rs +++ b/crates/omnyssh-core/src/ssh/pool.rs @@ -226,9 +226,17 @@ async fn run_host_poller( /// Wait for `delay`, but return early if a refresh signal is received. async fn wait_or_refresh(delay: Duration, refresh_rx: &mut mpsc::Receiver<()>) { + let sleep = tokio::time::sleep(delay); + tokio::pin!(sleep); tokio::select! { - _ = tokio::time::sleep(delay) => {} - _ = refresh_rx.recv() => {} + () = &mut sleep => {} + signal = refresh_rx.recv() => { + // `None` means every sender is gone. Returning on it would make the + // caller's loop spin, so serve out the delay instead. + if signal.is_none() { + sleep.await; + } + } } } @@ -472,6 +480,17 @@ mod tests { assert_eq!(start.elapsed(), Duration::from_secs(300)); } + #[tokio::test(start_paused = true)] + async fn wait_or_refresh_serves_out_its_delay_once_every_sender_is_gone() { + let (tx, mut rx) = mpsc::channel::<()>(4); + drop(tx); + + let start = tokio::time::Instant::now(); + wait_or_refresh(Duration::from_secs(300), &mut rx).await; + + assert_eq!(start.elapsed(), Duration::from_secs(300)); + } + #[tokio::test(start_paused = true)] async fn wait_or_refresh_still_returns_early() { let (tx, mut rx) = mpsc::channel::<()>(4); From a715322797abe0ec7d8a67e7acb5a30d01f2ec77 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 23:32:13 +0400 Subject: [PATCH 10/13] feat(core): add a TCP reachability monitoring mode Metrics need a POSIX shell, so a network appliance can only ever fail the metric round - and gets logged in to again on every cycle for the privilege. A host can now be watched by a plain TCP connect instead: no session, no authentication and no metrics, only reachability. The probe reports a status change rather than every cycle, backs off on failure the way the SSH poller does, and refuses a host behind ProxyJump instead of dialling the target address direct, where anything else on that address would answer for it. Both new fields are serde-defaulted and skipped when unset, so an existing hosts.toml is byte-identical and the SSH poller is untouched. --- crates/omnyssh-core/src/ssh/client.rs | 81 ++++++++++++++++ crates/omnyssh-core/src/ssh/pool.rs | 95 +++++++++++++++++- crates/omnyssh-core/tests/poll_backoff.rs | 111 +++++++++++++++++++++- crates/omnyssh-gui/src/dto.rs | 4 +- crates/omnyssh/src/app/host.rs | 4 +- 5 files changed, 291 insertions(+), 4 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/client.rs b/crates/omnyssh-core/src/ssh/client.rs index 20fa7b4..8f1c402 100644 --- a/crates/omnyssh-core/src/ssh/client.rs +++ b/crates/omnyssh-core/src/ssh/client.rs @@ -16,6 +16,28 @@ pub enum HostSource { Manual, } +/// How a host is watched on the dashboard. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Default)] +#[serde(rename_all = "snake_case")] +pub enum MonitorMode { + /// A live SSH session with shell metrics — the default. + #[default] + Ssh, + /// A plain TCP connect, with no login. For devices that answer SSH but have + /// no POSIX shell to collect metrics from, such as network appliances. + // The wire and the TUI form spell this differently; accept both, because a + // rejected value fails the whole file and takes every manual host with it. + #[serde(alias = "tcp", alias = "tcpPort")] + TcpPort, +} + +impl MonitorMode { + /// Lets the default stay out of `hosts.toml` entirely. + fn is_ssh(&self) -> bool { + matches!(self, Self::Ssh) + } +} + /// A single host entry used for SSH connections. /// /// Populated either from `~/.ssh/config` (via the parser) or from @@ -54,6 +76,12 @@ pub struct Host { /// Used to prevent duplicate entries when a SSH-config host is renamed. #[serde(skip_serializing_if = "Option::is_none")] pub original_ssh_host: Option, + /// How this host is watched. + #[serde(default, skip_serializing_if = "MonitorMode::is_ssh")] + pub monitoring: MonitorMode, + /// Port for the reachability probe. Falls back to `port` when unset. + #[serde(skip_serializing_if = "Option::is_none")] + pub monitor_port: Option, // ----------------------------------------------------------------------- // Auto SSH Key Setup metadata @@ -90,6 +118,8 @@ impl Default for Host { notes: None, source: HostSource::default(), original_ssh_host: None, + monitoring: MonitorMode::default(), + monitor_port: None, key_setup_date: None, password_auth_disabled: None, } @@ -109,3 +139,54 @@ pub enum ConnectionStatus { /// The last connection attempt failed with the given message. Failed(String), } + +#[cfg(test)] +mod tests { + use super::*; + + /// An existing `hosts.toml` predates the monitoring fields, and writing one + /// back must not add them — the file has to stay byte-identical for a host + /// nobody changed. + #[test] + fn the_monitoring_default_round_trips_without_touching_the_file() { + let host: Host = toml::from_str("name = \"web\"\nhostname = \"10.0.0.1\"\n") + .expect("a host without the monitoring fields still parses"); + assert_eq!(host.monitoring, MonitorMode::Ssh); + assert_eq!(host.monitor_port, None); + + let written = toml::to_string(&host).expect("serialize"); + assert!(!written.contains("monitoring"), "{written}"); + assert!(!written.contains("monitor_port"), "{written}"); + } + + /// The wire and the TUI form spell the mode differently, so a hand-edited + /// file is likely to carry either — and a rejected value fails the whole + /// file, taking every manual host with it. + #[test] + fn the_other_spellings_of_the_mode_are_accepted() { + for spelling in ["tcp_port", "tcp", "tcpPort"] { + let host: Host = toml::from_str(&format!( + "name = \"fw\"\nhostname = \"10.0.0.9\"\nmonitoring = \"{spelling}\"\n" + )) + .unwrap_or_else(|e| panic!("'{spelling}' should parse: {e}")); + assert_eq!(host.monitoring, MonitorMode::TcpPort); + } + } + + #[test] + fn a_reachability_host_persists_its_mode_and_probe_port() { + let host = Host { + name: String::from("fw"), + hostname: String::from("10.0.0.9"), + monitoring: MonitorMode::TcpPort, + monitor_port: Some(8443), + ..Host::default() + }; + + let written = toml::to_string(&host).expect("serialize"); + let read: Host = toml::from_str(&written).expect("deserialize"); + + assert_eq!(read.monitoring, MonitorMode::TcpPort); + assert_eq!(read.monitor_port, Some(8443)); + } +} diff --git a/crates/omnyssh-core/src/ssh/pool.rs b/crates/omnyssh-core/src/ssh/pool.rs index 4273c45..efaa759 100644 --- a/crates/omnyssh-core/src/ssh/pool.rs +++ b/crates/omnyssh-core/src/ssh/pool.rs @@ -13,11 +13,13 @@ use std::collections::HashMap; use std::time::{Duration, Instant}; +use tokio::net::TcpStream; use tokio::sync::mpsc; use tokio::task::JoinHandle; +use tokio::time; use crate::event::{CoreEvent, Metrics, ProcessInfo}; -use crate::ssh::client::{ConnectionStatus, Host}; +use crate::ssh::client::{ConnectionStatus, Host, MonitorMode}; use crate::ssh::metrics::{ parse_cpu_proc_stat, parse_cpu_top, parse_cpu_top_macos, parse_disk_df, parse_loadavg, parse_ram_free, parse_ram_vmstat, parse_top_processes, parse_uptime, @@ -126,6 +128,97 @@ impl PollManager { // --------------------------------------------------------------------------- async fn run_host_poller( + host: Host, + tx: mpsc::Sender, + poll_interval: Duration, + refresh_rx: mpsc::Receiver<()>, +) { + match host.monitoring { + MonitorMode::Ssh => run_ssh_poller(host, tx, poll_interval, refresh_rx).await, + MonitorMode::TcpPort => run_tcp_poller(host, tx, poll_interval, refresh_rx).await, + } +} + +/// How long a reachability probe waits for the port to answer. +const TCP_PROBE_TIMEOUT: Duration = Duration::from_secs(5); + +/// Reachability-only poller: one TCP connect per cycle, no SSH session and no +/// authentication, so a device that cannot serve metrics is never logged in to. +/// Emits status only — a host in this mode reports no metrics. +async fn run_tcp_poller( + host: Host, + tx: mpsc::Sender, + poll_interval: Duration, + mut refresh_rx: mpsc::Receiver<()>, +) { + // A bare TCP dial cannot traverse a bastion, and probing the target address + // direct would silently report on whatever else answers it. + if host.proxy_jump.is_some() { + send_status( + &tx, + &host.name, + ConnectionStatus::Failed(String::from( + "a port check cannot reach a host behind ProxyJump - use SSH monitoring", + )), + ) + .await; + return; + } + + let port = host.monitor_port.filter(|&p| p != 0).unwrap_or(host.port); + let addr = format!("{}:{}", host.hostname, port); + let mut backoff = BackoffState::new(); + let mut last: Option = None; + + loop { + if last.is_none() + && tx + .send(CoreEvent::HostStatusChanged( + host.name.clone(), + ConnectionStatus::Connecting, + )) + .await + .is_err() + { + return; // App has shut down. + } + + let status = match time::timeout(TCP_PROBE_TIMEOUT, TcpStream::connect(&addr)).await { + Ok(Ok(_)) => ConnectionStatus::Connected, + Ok(Err(e)) => ConnectionStatus::Failed(e.to_string()), + Err(_) => ConnectionStatus::Failed(format!("no answer from {addr}")), + }; + let reachable = matches!(status, ConnectionStatus::Connected); + + // Only on a change: re-announcing every cycle flickers the card between + // reachable and checking and re-buckets the host in the status bar. + if last.as_ref() != Some(&status) { + if tx + .send(CoreEvent::HostStatusChanged( + host.name.clone(), + status.clone(), + )) + .await + .is_err() + { + return; + } + last = Some(status); + } + + if reachable { + backoff.reset(); + wait_or_refresh(poll_interval, &mut refresh_rx).await; + } else { + // Same restraint as the SSH poller: a device that is down should not + // be re-dialled on every refresh tick. + let delay = backoff.next_delay().max(poll_interval); + wait_backoff(delay, &mut refresh_rx).await; + } + } +} + +async fn run_ssh_poller( host: Host, tx: mpsc::Sender, poll_interval: Duration, diff --git a/crates/omnyssh-core/tests/poll_backoff.rs b/crates/omnyssh-core/tests/poll_backoff.rs index b963759..cfac70f 100644 --- a/crates/omnyssh-core/tests/poll_backoff.rs +++ b/crates/omnyssh-core/tests/poll_backoff.rs @@ -12,7 +12,7 @@ use tokio::net::TcpListener; use tokio::sync::mpsc; use omnyssh_core::event::CoreEvent; -use omnyssh_core::ssh::client::Host; +use omnyssh_core::ssh::client::{ConnectionStatus, Host, MonitorMode}; use omnyssh_core::ssh::pool::PollManager; /// Counts connections, answering none of them. @@ -101,3 +101,112 @@ async fn an_unreachable_host_keeps_retrying_on_its_own_schedule() { // A host that stops answering is retried, not abandoned. assert!(retried, "the poller stopped retrying an unreachable host"); } + +#[tokio::test(start_paused = true)] +async fn a_reachability_host_is_probed_without_an_ssh_session() { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let port = listener.local_addr().expect("local addr").port(); + tokio::spawn(async move { while listener.accept().await.is_ok() {} }); + + let host = Host { + monitoring: MonitorMode::TcpPort, + // The probe port wins over the SSH port, which is deliberately dead here. + port: 1, + monitor_port: Some(port), + ..unreachable_host(port) + }; + let (tx, mut rx) = mpsc::channel(64); + let manager = PollManager::start(vec![host], tx, Duration::from_secs(30)); + + // Bounded: the poller never closes the channel, so an unbounded wait would + // hang instead of failing when the expected status stops arriving. + let reachable = tokio::time::timeout(Duration::from_secs(60), async { + while let Some(event) = rx.recv().await { + if let CoreEvent::HostStatusChanged(_, ConnectionStatus::Connected) = event { + return true; + } + } + false + }) + .await; + assert_eq!( + reachable, + Ok(true), + "the probe never reported the port as reachable" + ); + + // And over the cycles that follow, it reports status only — metrics would be + // invented. Elapsing without a metrics event is the pass. + let stray_metrics = tokio::time::timeout(Duration::from_secs(120), async { + while let Some(event) = rx.recv().await { + if matches!(event, CoreEvent::MetricsUpdate(..)) { + return true; + } + } + false + }) + .await; + manager.shutdown(); + + assert_ne!( + stray_metrics, + Ok(true), + "a tcp-probed host must not report metrics" + ); +} + +#[tokio::test(start_paused = true)] +async fn a_reachability_host_reports_a_closed_port_as_failed() { + // Port 1 is closed and cannot be re-bound by anything else mid-test. + let port = 1; + + let host = Host { + monitoring: MonitorMode::TcpPort, + ..unreachable_host(port) + }; + let (tx, mut rx) = mpsc::channel(64); + let manager = PollManager::start(vec![host], tx, Duration::from_secs(30)); + + let failed = tokio::time::timeout(Duration::from_secs(60), async { + while let Some(event) = rx.recv().await { + if let CoreEvent::HostStatusChanged(_, ConnectionStatus::Failed(_)) = event { + return true; + } + } + false + }) + .await; + manager.shutdown(); + + assert_eq!( + failed, + Ok(true), + "a closed port was not reported as unreachable" + ); +} + +#[tokio::test(start_paused = true)] +async fn a_reachability_host_behind_a_bastion_says_so_instead_of_probing_direct() { + let host = Host { + monitoring: MonitorMode::TcpPort, + proxy_jump: Some(String::from("bastion")), + ..unreachable_host(22) + }; + let (tx, mut rx) = mpsc::channel(64); + let manager = PollManager::start(vec![host], tx, Duration::from_secs(30)); + + let explained = tokio::time::timeout(Duration::from_secs(60), async { + while let Some(event) = rx.recv().await { + if let CoreEvent::HostStatusChanged(_, ConnectionStatus::Failed(message)) = event { + // Probing the target address direct would report on whatever + // else answers it, which is worse than saying nothing. + return message.contains("ProxyJump"); + } + } + false + }) + .await; + manager.shutdown(); + + assert_eq!(explained, Ok(true), "a bastion host was probed direct"); +} diff --git a/crates/omnyssh-gui/src/dto.rs b/crates/omnyssh-gui/src/dto.rs index 2df64f1..9007f30 100644 --- a/crates/omnyssh-gui/src/dto.rs +++ b/crates/omnyssh-gui/src/dto.rs @@ -9,7 +9,7 @@ use omnyssh_core::config::snippets::{Snippet, SnippetScope}; use omnyssh_core::event::{ DetectedService, MetricValue, Metrics, ProcessInfo, ServiceKind, ServiceMetric, }; -use omnyssh_core::ssh::client::{ConnectionStatus, Host, HostSource}; +use omnyssh_core::ssh::client::{ConnectionStatus, Host, HostSource, MonitorMode}; use omnyssh_core::ssh::key_setup::KeySetupStep; use omnyssh_core::ssh::sftp::FileEntry; use omnyssh_core::update::UpdateInfo; @@ -285,6 +285,8 @@ impl From for Host { notes: non_empty(dto.notes), source: HostSource::Manual, original_ssh_host: None, + monitoring: MonitorMode::default(), + monitor_port: None, key_setup_date: None, password_auth_disabled: None, } diff --git a/crates/omnyssh/src/app/host.rs b/crates/omnyssh/src/app/host.rs index f730332..ad5461a 100644 --- a/crates/omnyssh/src/app/host.rs +++ b/crates/omnyssh/src/app/host.rs @@ -4,7 +4,7 @@ use std::time::Duration; use super::*; -use omnyssh_core::ssh::client::HostSource; +use omnyssh_core::ssh::client::{HostSource, MonitorMode}; // --------------------------------------------------------------------------- // Host form (used in Add / Edit popups) @@ -175,6 +175,8 @@ impl HostForm { notes, source, original_ssh_host: None, + monitoring: MonitorMode::default(), + monitor_port: None, key_setup_date: None, password_auth_disabled: None, }) From ae18b2f8ffb1d0eedaf5f49c6863e839c4082601 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 23:32:29 +0400 Subject: [PATCH 11/13] feat(tui): expose the monitoring mode on the host form and card The form takes 'ssh', 'tcp' or 'tcp:PORT'; the card and the detail view show the probe result, naming the port it dialled, instead of metric rows for numbers that were never collected. An edit now restarts the pollers, without which a mode change was written to disk and then ignored until the next launch, and the form sizes itself to its fields so the last one survives a short terminal. --- crates/omnyssh/src/app/host.rs | 80 ++++++++++++++++++++++++++-- crates/omnyssh/src/ui/card.rs | 30 ++++++++++- crates/omnyssh/src/ui/dashboard.rs | 2 + crates/omnyssh/src/ui/detail_view.rs | 17 +++++- crates/omnyssh/src/ui/popup.rs | 10 +++- 5 files changed, 131 insertions(+), 8 deletions(-) diff --git a/crates/omnyssh/src/app/host.rs b/crates/omnyssh/src/app/host.rs index ad5461a..0545897 100644 --- a/crates/omnyssh/src/app/host.rs +++ b/crates/omnyssh/src/app/host.rs @@ -20,8 +20,33 @@ pub const FORM_FIELD_LABELS: &[&str] = &[ "Password (optional)", "Tags (comma-sep)", "Notes", + "Monitoring (ssh | tcp | tcp:PORT)", ]; +/// Renders a host's monitoring mode back into its form field. +fn monitoring_value(host: &Host) -> String { + match (host.monitoring, host.monitor_port) { + (MonitorMode::Ssh, _) => String::new(), + (MonitorMode::TcpPort, Some(port)) => format!("tcp:{port}"), + (MonitorMode::TcpPort, None) => String::from("tcp"), + } +} + +/// Parses the monitoring field: empty or `ssh` keeps the SSH poller, `tcp` +/// probes the host's SSH port, `tcp:PORT` probes another one. +fn parse_monitoring(value: &str) -> Result<(MonitorMode, Option), String> { + match value { + "" | "ssh" => Ok((MonitorMode::Ssh, None)), + "tcp" => Ok((MonitorMode::TcpPort, None)), + other => other + .strip_prefix("tcp:") + .and_then(|p| p.parse::().ok()) + .filter(|&p| p != 0) + .map(|p| (MonitorMode::TcpPort, Some(p))) + .ok_or_else(|| format!("Monitoring must be 'ssh', 'tcp' or 'tcp:PORT', got '{other}'")), + } +} + /// A single editable text field in the host form. #[derive(Debug, Clone, Default)] pub struct FormField { @@ -91,6 +116,7 @@ impl HostForm { form.fields[5] = FormField::with_value(host.password.as_deref().unwrap_or("")); form.fields[6] = FormField::with_value(host.tags.join(", ")); form.fields[7] = FormField::with_value(host.notes.as_deref().unwrap_or("")); + form.fields[8] = FormField::with_value(monitoring_value(host)); form } @@ -163,6 +189,8 @@ impl HostForm { } }; + let (monitoring, monitor_port) = parse_monitoring(self.fields[8].value.trim())?; + Ok(Host { name, hostname, @@ -175,8 +203,8 @@ impl HostForm { notes, source, original_ssh_host: None, - monitoring: MonitorMode::default(), - monitor_port: None, + monitoring, + monitor_port, key_setup_date: None, password_auth_disabled: None, }) @@ -492,6 +520,21 @@ impl App { } self.save_manual_hosts().await; + + // The edit can change the address, the port or the monitoring + // mode, none of which a running poller picks up. + { + let state = self.state.read().await; + if let Some(old) = self.poll_manager.take() { + old.shutdown(); + } + self.poll_manager = Some(PollManager::start( + state.hosts.clone(), + self.core_tx.clone(), + Duration::from_secs(30), + )); + } + let state = self.state.read().await; self.view.host_list.rebuild_filter( &state.hosts, @@ -546,9 +589,10 @@ impl App { #[cfg(test)] mod tests { + use super::*; - /// Builds a host form from the 8 field values, in `FORM_FIELD_LABELS` order. + /// Builds a host form from the field values, in `FORM_FIELD_LABELS` order. fn host_form(values: [&str; 8]) -> HostForm { let mut form = HostForm::empty(); for (i, v) in values.iter().enumerate() { @@ -557,6 +601,36 @@ mod tests { form } + #[test] + fn the_monitoring_field_round_trips_through_the_form() { + for (text, mode, port) in [ + ("", MonitorMode::Ssh, None), + ("ssh", MonitorMode::Ssh, None), + ("tcp", MonitorMode::TcpPort, None), + ("tcp:8443", MonitorMode::TcpPort, Some(8443)), + ] { + let parsed = parse_monitoring(text).expect("valid monitoring value"); + assert_eq!(parsed, (mode, port), "parsing '{text}'"); + + let host = Host { + monitoring: mode, + monitor_port: port, + ..Host::default() + }; + assert_eq!(parse_monitoring(&monitoring_value(&host)), Ok((mode, port))); + } + } + + #[test] + fn an_unusable_monitoring_value_is_rejected() { + for text in ["tcp:0", "tcp:99999", "http", "tcp:"] { + assert!( + parse_monitoring(text).is_err(), + "'{text}' should be rejected" + ); + } + } + // --- HostForm::to_host (P0.2) ----------------------------------------- #[test] diff --git a/crates/omnyssh/src/ui/card.rs b/crates/omnyssh/src/ui/card.rs index ab6a388..8ab2584 100644 --- a/crates/omnyssh/src/ui/card.rs +++ b/crates/omnyssh/src/ui/card.rs @@ -14,7 +14,7 @@ use ratatui::{ use crate::ui::theme::threshold_color; use crate::ui::theme::Theme; use omnyssh_core::event::{DetectedService, Metrics, ServiceKind}; -use omnyssh_core::ssh::client::ConnectionStatus; +use omnyssh_core::ssh::client::{ConnectionStatus, MonitorMode}; // --------------------------------------------------------------------------- // Card dimensions (kept in sync with dashboard.rs column calculation) @@ -54,6 +54,25 @@ pub struct CardData<'a> { pub status: Option<&'a ConnectionStatus>, /// Detected services. pub services: Option<&'a [DetectedService]>, + /// How the host is watched. A reachability host has no metrics to show. + pub monitoring: MonitorMode, + /// Port the reachability probe dials, when it is not the host's own. + pub monitor_port: Option, +} + +/// The reachability line shown in place of the metric rows. Naming the probed +/// port matters: a green line for port 8443 says nothing about SSH on 22. +fn reachability_line(status: Option<&ConnectionStatus>, port: Option) -> (String, Color) { + let (state, color) = match status { + Some(ConnectionStatus::Connected) => ("reachable", Color::Green), + Some(ConnectionStatus::Failed(_)) => ("unreachable", Color::Red), + _ => ("checking", Color::DarkGray), + }; + let text = match port { + Some(p) => format!("─── {state} :{p} ───"), + None => format!("─── {state} ───"), + }; + (text, color) } /// Render a single server card into `rect`. @@ -162,7 +181,14 @@ pub fn render_card( Some(ConnectionStatus::Failed(_)) | Some(ConnectionStatus::Unknown) | None ) && metrics.is_none(); - if is_offline { + if data.monitoring != MonitorMode::Ssh { + // Reachability host: no metrics exist, so the tiles would be a fiction. + let (text, color) = reachability_line(status, data.monitor_port); + frame.render_widget( + Paragraph::new(Line::from(Span::styled(text, Style::default().fg(color)))), + rows[1], + ); + } else if is_offline { // Rows 1-2: offline message frame.render_widget( Paragraph::new(Line::from(Span::styled( diff --git a/crates/omnyssh/src/ui/dashboard.rs b/crates/omnyssh/src/ui/dashboard.rs index cd1fa67..c2c61a4 100644 --- a/crates/omnyssh/src/ui/dashboard.rs +++ b/crates/omnyssh/src/ui/dashboard.rs @@ -266,6 +266,8 @@ fn render_grid(frame: &mut Frame, area: Rect, state: &AppState, view: &ViewState metrics, status, services: state.services.get(&host.name).map(|s| s.as_slice()), + monitoring: host.monitoring, + monitor_port: host.monitor_port, }, is_selected, &view.theme, diff --git a/crates/omnyssh/src/ui/detail_view.rs b/crates/omnyssh/src/ui/detail_view.rs index 2c4b644..1ff0151 100644 --- a/crates/omnyssh/src/ui/detail_view.rs +++ b/crates/omnyssh/src/ui/detail_view.rs @@ -16,7 +16,7 @@ use crate::app::{AppAction, AppState, SnippetPopup, ViewState}; use crate::ui::theme::threshold_color; use crate::ui::theme::Theme; use omnyssh_core::event::{DetectedService, Metrics, ServiceKind}; -use omnyssh_core::ssh::client::ConnectionStatus; +use omnyssh_core::ssh::client::{ConnectionStatus, MonitorMode}; // --------------------------------------------------------------------------- // Render @@ -111,7 +111,13 @@ pub fn render(frame: &mut Frame, area: Rect, state: &AppState, view: &ViewState) // Separator render_separator(frame, sections[2], inner.width, &view.theme); - // Metrics + // Metrics — a reachability host has none, and the last SSH sample it may still + // carry is not current. + let metrics = if host.monitoring == MonitorMode::Ssh { + metrics + } else { + None + }; render_metrics_alerts(frame, sections[3], metrics, &view.theme); // Separator @@ -216,6 +222,13 @@ fn render_metrics_column(frame: &mut Frame, area: Rect, metrics: Option<&Metrics .add_modifier(Modifier::BOLD), ))]; + if metrics.is_none() { + lines.push(Line::from(Span::styled( + " unavailable", + Style::default().fg(theme.text_secondary), + ))); + } + if let Some(m) = metrics { // CPU line with bar if let Some(cpu) = m.cpu_percent { diff --git a/crates/omnyssh/src/ui/popup.rs b/crates/omnyssh/src/ui/popup.rs index f63eb8b..3c73014 100644 --- a/crates/omnyssh/src/ui/popup.rs +++ b/crates/omnyssh/src/ui/popup.rs @@ -316,7 +316,15 @@ pub fn render_help(frame: &mut Frame, theme: &Theme) { /// `title` is either `"Add Host"` or `"Edit Host"`. pub fn render_host_form(frame: &mut Frame, form: &HostForm, title: &str, theme: &Theme) { // Taller popup to fit all fields. - let area = centred_rect(70, 80, frame.area()); + // One row for each label and input, plus top padding, the hint and its spacer, + // and the border. Sizing to that rather than to a fixed share of the screen + // keeps the last field on screen when the terminal is short. + let frame_area = frame.area(); + let needed = u32::from(FORM_FIELD_LABELS.len() as u16) * 2 + 5; + let percent = (needed * 100) + .div_ceil(u32::from(frame_area.height.max(1))) + .clamp(50, 100) as u16; + let area = centred_rect(70, percent, frame_area); frame.render_widget(Clear, area); let block = Block::default() From 74218770cd03fe5e66717495e08dc83147d39809 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 23:32:48 +0400 Subject: [PATCH 12/13] feat(gui): expose the monitoring mode in the host editor and card Resolves the CONTRACT GAP for the per-host monitoring mode: HostDto and HostInputDto carry it, MonitorModeDto is the wire enum, and bindings.ts is regenerated. No new command and no new event - ConnectionStatusDto already carries the probe result. An omitted mode on save means unchanged, not back to SSH, and the status bar ignores the metrics a reachability host stopped refreshing, which would otherwise pin it in the alert bucket until the app restarted. --- crates/omnyssh-gui/src/commands/hosts.rs | 54 +++++++++++++-- crates/omnyssh-gui/src/dto.rs | 49 ++++++++++++- crates/omnyssh-gui/ui/src/lib/bindings.ts | 9 ++- .../omnyssh-gui/ui/src/lib/ipc/router.test.ts | 5 +- .../ui/src/lib/screens/Dashboard.svelte | 11 ++- .../ui/src/lib/screens/HostEditor.svelte | 24 +++++++ .../ui/src/lib/screens/hostForm.test.ts | 68 ++++++++++++++++++- .../ui/src/lib/screens/hostForm.ts | 39 +++++++++-- .../ui/src/lib/screens/serverCard.test.ts | 40 ++++++++++- .../ui/src/lib/screens/serverCard.ts | 20 ++++++ .../ui/src/lib/stores/hostSummary.test.ts | 26 ++++++- .../ui/src/lib/stores/hostSummary.ts | 5 +- .../ui/src/lib/stores/palette.test.ts | 1 + 13 files changed, 331 insertions(+), 20 deletions(-) diff --git a/crates/omnyssh-gui/src/commands/hosts.rs b/crates/omnyssh-gui/src/commands/hosts.rs index 302a267..7a6ef40 100644 --- a/crates/omnyssh-gui/src/commands/hosts.rs +++ b/crates/omnyssh-gui/src/commands/hosts.rs @@ -79,14 +79,22 @@ pub async fn delete_host(name: String) -> Result<(), CommandError> { /// Upsert `input` into the manual host list by name. A new name appends; an existing /// name is an in-place edit that **preserves every field the edit form cannot observe** /// — password, identity file, and proxy jump (the outbound `HostDto` omits all three, -/// §3.4, so the form leaves them blank on edit), plus key-setup metadata and the -/// SSH-config rename origin. Editing e.g. notes therefore never drops a stored secret -/// or a recorded key setup. A provided secret still overwrites the old one. +/// §3.4, so the form leaves them blank on edit), plus key-setup metadata, the +/// SSH-config rename origin, and a monitoring mode the payload left out. Editing +/// e.g. notes therefore never drops a stored secret or a recorded key setup. A +/// provided secret still overwrites the old one. fn upsert(hosts: &mut Vec, input: HostInputDto) { + // An omitted monitoring mode means "unchanged", not "back to SSH" — losing it + // would silently start logging in to a device chosen for reachability only. + let monitoring_given = input.monitoring.is_some(); let mut host = Host::from(input); match hosts.iter().position(|h| h.name == host.name) { Some(i) => { let existing = &hosts[i]; + if !monitoring_given { + host.monitoring = existing.monitoring; + host.monitor_port = existing.monitor_port; + } host.password = host.password.or_else(|| existing.password.clone()); host.identity_file = host .identity_file @@ -128,7 +136,8 @@ async fn persist(mutate: impl FnOnce(&mut Vec) + Send + 'static) -> Result #[cfg(test)] mod tests { use super::*; - use omnyssh_core::ssh::client::HostSource; + use crate::dto::MonitorModeDto; + use omnyssh_core::ssh::client::{HostSource, MonitorMode}; fn input(name: &str) -> HostInputDto { HostInputDto { @@ -141,6 +150,8 @@ mod tests { proxy_jump: None, tags: vec![], notes: None, + monitoring: None, + monitor_port: None, } } @@ -196,6 +207,41 @@ mod tests { assert_eq!(h.original_ssh_host.as_deref(), Some("web-old")); } + #[test] + fn upsert_keeps_a_monitoring_mode_the_payload_left_out() { + let mut hosts = vec![Host { + name: "fw".to_string(), + monitoring: MonitorMode::TcpPort, + monitor_port: Some(8443), + source: HostSource::Manual, + ..Host::default() + }]; + + upsert(&mut hosts, input("fw")); + + // Silently reverting to SSH would start logging in to a device the user + // deliberately put on a reachability probe. + assert_eq!(hosts[0].monitoring, MonitorMode::TcpPort); + assert_eq!(hosts[0].monitor_port, Some(8443)); + } + + #[test] + fn upsert_applies_a_monitoring_mode_the_payload_carries() { + let mut hosts = vec![Host { + name: "fw".to_string(), + monitoring: MonitorMode::TcpPort, + monitor_port: Some(8443), + source: HostSource::Manual, + ..Host::default() + }]; + + let mut back_to_ssh = input("fw"); + back_to_ssh.monitoring = Some(MonitorModeDto::Ssh); + upsert(&mut hosts, back_to_ssh); + + assert_eq!(hosts[0].monitoring, MonitorMode::Ssh); + } + #[test] fn upsert_overwrites_a_secret_when_a_new_one_is_provided() { let mut hosts = vec![Host { diff --git a/crates/omnyssh-gui/src/dto.rs b/crates/omnyssh-gui/src/dto.rs index 9007f30..71bd7e5 100644 --- a/crates/omnyssh-gui/src/dto.rs +++ b/crates/omnyssh-gui/src/dto.rs @@ -22,6 +22,33 @@ pub enum HostSourceDto { Manual, } +/// How a host is watched, mirrors `omnyssh_core::ssh::client::MonitorMode` +/// (tech-gui.md §4.1). `tcpPort` means reachability only — no login, no metrics. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, specta::Type)] +#[serde(rename_all = "camelCase")] +pub enum MonitorModeDto { + Ssh, + TcpPort, +} + +impl From for MonitorModeDto { + fn from(mode: MonitorMode) -> Self { + match mode { + MonitorMode::Ssh => Self::Ssh, + MonitorMode::TcpPort => Self::TcpPort, + } + } +} + +impl From for MonitorMode { + fn from(mode: MonitorModeDto) -> Self { + match mode { + MonitorModeDto::Ssh => Self::Ssh, + MonitorModeDto::TcpPort => Self::TcpPort, + } + } +} + /// A host as the frontend sees it — password and private-key material omitted /// (tech-gui.md §3.4). `hasKey` reports whether an identity file is configured; /// the key path itself never crosses the boundary. @@ -39,6 +66,9 @@ pub struct HostDto { pub has_key: bool, #[serde(skip_serializing_if = "Option::is_none")] pub password_auth_disabled: Option, + pub monitoring: MonitorModeDto, + #[serde(skip_serializing_if = "Option::is_none")] + pub monitor_port: Option, } /// Inbound host form payload for `save_host` (tech-gui.md §4.1, Stage 4.1). Builds a @@ -64,6 +94,10 @@ pub struct HostInputDto { pub tags: Vec, #[serde(default)] pub notes: Option, + #[serde(default)] + pub monitoring: Option, + #[serde(default)] + pub monitor_port: Option, } /// Live connection state for a host (tech-gui.md §4.1). Internally tagged so the @@ -258,6 +292,8 @@ impl From<&Host> for HostDto { source: (&host.source).into(), has_key: host.identity_file.is_some(), password_auth_disabled: host.password_auth_disabled, + monitoring: host.monitoring.into(), + monitor_port: host.monitor_port, } } } @@ -273,6 +309,7 @@ impl From for Host { // The frontend already trims; collapse an exact-empty string to `None` as a // last guard. Password is not trimmed — its bytes are preserved verbatim. let non_empty = |s: Option| s.filter(|v| !v.is_empty()); + let monitoring: MonitorMode = dto.monitoring.map(Into::into).unwrap_or_default(); Host { name: dto.name, hostname: dto.hostname, @@ -285,8 +322,12 @@ impl From for Host { notes: non_empty(dto.notes), source: HostSource::Manual, original_ssh_host: None, - monitoring: MonitorMode::default(), - monitor_port: None, + monitoring, + // Port 0 is not dialable, and an SSH host has nothing to probe: drop + // both, so a later mode switch cannot inherit a stale target. + monitor_port: dto + .monitor_port + .filter(|&p| p != 0 && monitoring == MonitorMode::TcpPort), key_setup_date: None, password_auth_disabled: None, } @@ -534,6 +575,8 @@ mod tests { proxy_jump: Some("bastion".to_string()), tags: vec!["prod".to_string()], notes: Some("primary".to_string()), + monitoring: None, + monitor_port: None, } } @@ -585,6 +628,8 @@ mod tests { proxy_jump: Some(String::new()), tags: vec![], notes: Some(String::new()), + monitoring: None, + monitor_port: None, }); assert!(host.identity_file.is_none()); assert!(host.password.is_none()); diff --git a/crates/omnyssh-gui/ui/src/lib/bindings.ts b/crates/omnyssh-gui/ui/src/lib/bindings.ts index b7d4a1c..6c3af67 100644 --- a/crates/omnyssh-gui/ui/src/lib/bindings.ts +++ b/crates/omnyssh-gui/ui/src/lib/bindings.ts @@ -435,7 +435,7 @@ export type FilePreview = { sessionId: number; path: string; content: string } * (tech-gui.md §3.4). `hasKey` reports whether an identity file is configured; * the key path itself never crosses the boundary. */ -export type HostDto = { name: string; hostname: string; user: string; port: number; tags: string[]; notes?: string | null; source: HostSourceDto; hasKey: boolean; passwordAuthDisabled?: boolean | null } +export type HostDto = { name: string; hostname: string; user: string; port: number; tags: string[]; notes?: string | null; source: HostSourceDto; hasKey: boolean; passwordAuthDisabled?: boolean | null; monitoring: MonitorModeDto; monitorPort?: number | null } /** * Inbound host form payload for `save_host` (tech-gui.md §4.1, Stage 4.1). Builds a * **manual** `Host` — SSH-config hosts are read-only imports and are never saved. @@ -443,7 +443,7 @@ export type HostDto = { name: string; hostname: string; user: string; port: numb * travel back out: the outbound `HostDto` omits both (§3.4). Inbound only, so it * derives `Deserialize` (not `Serialize`). */ -export type HostInputDto = { name: string; hostname: string; user: string; port: number; identityFile?: string | null; password?: string | null; proxyJump?: string | null; tags: string[]; notes?: string | null } +export type HostInputDto = { name: string; hostname: string; user: string; port: number; identityFile?: string | null; password?: string | null; proxyJump?: string | null; tags: string[]; notes?: string | null; monitoring?: MonitorModeDto | null; monitorPort?: number | null } /** * Host origin, mirrors `omnyssh_core::ssh::client::HostSource`. */ @@ -492,6 +492,11 @@ export type MetricsDto = { cpuPercent?: number | null; ramPercent?: number | nul * A fresh metrics sample for a host (tech-gui.md §4.3). */ export type MetricsUpdated = { hostName: string; metrics: MetricsDto } +/** + * How a host is watched, mirrors `omnyssh_core::ssh::client::MonitorMode` + * (tech-gui.md §4.1). `tcpPort` means reachability only — no login, no metrics. + */ +export type MonitorModeDto = "ssh" | "tcpPort" /** * A single process in the "top processes" panel (tech-gui.md §4.1). */ diff --git a/crates/omnyssh-gui/ui/src/lib/ipc/router.test.ts b/crates/omnyssh-gui/ui/src/lib/ipc/router.test.ts index 25245ea..526dabf 100644 --- a/crates/omnyssh-gui/ui/src/lib/ipc/router.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/ipc/router.test.ts @@ -43,7 +43,8 @@ describe('ipc event router', () => { port: 22, tags: [], source: 'manual', - hasKey: false + hasKey: false, + monitoring: 'ssh' } ]; @@ -110,7 +111,7 @@ describe('ipc event router', () => { applyServicesDetected({ hostName: 'web-2', services: [{ kind: 'docker', metrics: [] }] }); applyHostsLoaded([ - { name: 'web-1', hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false } + { name: 'web-1', hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false, monitoring: 'ssh' } ]); expect(get(statuses).has('web-2')).toBe(false); diff --git a/crates/omnyssh-gui/ui/src/lib/screens/Dashboard.svelte b/crates/omnyssh-gui/ui/src/lib/screens/Dashboard.svelte index 6d6aa5b..9406fba 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/Dashboard.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/Dashboard.svelte @@ -281,8 +281,15 @@ - - {#if card.offline} + + {#if card.reachability} +
+ {card.reachability}{card.host.monitorPort ? ` · port ${card.host.monitorPort}` : ''} +
+ {:else if card.offline}
offline
{:else}
diff --git a/crates/omnyssh-gui/ui/src/lib/screens/HostEditor.svelte b/crates/omnyssh-gui/ui/src/lib/screens/HostEditor.svelte index 3bb830e..50a9fb2 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/HostEditor.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/HostEditor.svelte @@ -135,6 +135,30 @@ +
+ + {#if fields.monitoring === 'tcpPort'} + + {/if} +
+ {#if fields.monitoring === 'tcpPort'} +

Checks the port only — no login, and no metrics on the card.

+ {/if} + {#if error}

{error}

{/if} diff --git a/crates/omnyssh-gui/ui/src/lib/screens/hostForm.test.ts b/crates/omnyssh-gui/ui/src/lib/screens/hostForm.test.ts index 5c5fe4c..b26a61a 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/hostForm.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/screens/hostForm.test.ts @@ -15,6 +15,7 @@ function host(partial: Partial): HostDto { tags: [], source: 'manual', hasKey: false, + monitoring: 'ssh', ...partial }; } @@ -124,7 +125,72 @@ describe('formFromHost', () => { identityFile: undefined, password: undefined, tags: ['ops'], - notes: 'x' + notes: 'x', + monitoring: 'ssh', + monitorPort: undefined }); }); }); + +describe('formToInput — monitoring mode', () => { + it('defaults to ssh and sends no probe port', () => { + const result = formToInput({ ...emptyForm(), name: 'web', hostname: '10.0.0.1' }); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.input.monitoring).toBe('ssh'); + expect(result.input.monitorPort).toBeUndefined(); + } + }); + + it('carries a probe port for a reachability host', () => { + const result = formToInput({ + ...emptyForm(), + name: 'fw', + hostname: '10.0.0.9', + monitoring: 'tcpPort', + monitorPort: '8443' + }); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.input.monitoring).toBe('tcpPort'); + expect(result.input.monitorPort).toBe(8443); + } + }); + + it('falls back to the host port when the probe port is blank', () => { + const result = formToInput({ ...emptyForm(), name: 'fw', hostname: '10.0.0.9', monitoring: 'tcpPort' }); + expect(result.ok).toBe(true); + if (result.ok) expect(result.input.monitorPort).toBeUndefined(); + }); + + it('rejects an out-of-range probe port', () => { + for (const monitorPort of ['0', '99999', 'ssh']) { + const result = formToInput({ + ...emptyForm(), + name: 'fw', + hostname: '10.0.0.9', + monitoring: 'tcpPort', + monitorPort + }); + expect(result.ok).toBe(false); + } + }); + + it('ignores a probe port left over from switching back to ssh', () => { + const result = formToInput({ + ...emptyForm(), + name: 'web', + hostname: '10.0.0.1', + monitoring: 'ssh', + monitorPort: '8443' + }); + expect(result.ok).toBe(true); + if (result.ok) expect(result.input.monitorPort).toBeUndefined(); + }); + + it('round-trips a reachability host through the edit form', () => { + const fields = formFromHost(host({ monitoring: 'tcpPort', monitorPort: 8443 })); + expect(fields.monitoring).toBe('tcpPort'); + expect(fields.monitorPort).toBe('8443'); + }); +}); diff --git a/crates/omnyssh-gui/ui/src/lib/screens/hostForm.ts b/crates/omnyssh-gui/ui/src/lib/screens/hostForm.ts index eeb8788..7f32567 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/hostForm.ts +++ b/crates/omnyssh-gui/ui/src/lib/screens/hostForm.ts @@ -3,7 +3,7 @@ // validation mirrors the TUI's `HostForm::to_host` (crates/omnyssh/src/app/host.rs) // so both frontends produce the same `hosts.toml` shape and error messages. -import type { HostDto, HostInputDto } from '$lib/bindings'; +import type { HostDto, HostInputDto, MonitorModeDto } from '$lib/bindings'; /** The editable form fields — all raw text (tags are comma-separated, port a string). */ export interface HostFormFields { @@ -15,12 +15,26 @@ export interface HostFormFields { password: string; tags: string; notes: string; + monitoring: MonitorModeDto; + /** Probe port; blank means "the host's SSH port". Only read for `tcpPort`. */ + monitorPort: string; } export function emptyForm(): HostFormFields { // Port pre-seeded to the SSH default; user blank (placeholder shows `root`, the // default the validation applies when it is left empty). - return { name: '', hostname: '', user: '', port: '22', identityFile: '', password: '', tags: '', notes: '' }; + return { + name: '', + hostname: '', + user: '', + port: '22', + identityFile: '', + password: '', + tags: '', + notes: '', + monitoring: 'ssh', + monitorPort: '' + }; } /** Seed the edit form from a `HostDto`. `identityFile`/`password` are intentionally @@ -35,7 +49,9 @@ export function formFromHost(h: HostDto): HostFormFields { identityFile: '', password: '', tags: h.tags.join(', '), - notes: h.notes ?? '' + notes: h.notes ?? '', + monitoring: h.monitoring, + monitorPort: h.monitorPort == null ? '' : String(h.monitorPort) }; } @@ -72,6 +88,19 @@ export function formToInput(f: HostFormFields): HostFormResult { port = Number(portRaw); } + // Only meaningful for a reachability host; an SSH host never carries a probe port. + let monitorPort: number | undefined; + const monitorPortRaw = f.monitorPort.trim(); + if (f.monitoring === 'tcpPort' && monitorPortRaw !== '') { + if (!/^\+?\d+$/.test(monitorPortRaw) || Number(monitorPortRaw) < 1 || Number(monitorPortRaw) > 65535) { + return { + ok: false, + error: `Probe port must be a number between 1 and 65535, got '${monitorPortRaw}'` + }; + } + monitorPort = Number(monitorPortRaw); + } + const identityFile = f.identityFile.trim(); const password = f.password.trim(); const notes = f.notes.trim(); @@ -86,7 +115,9 @@ export function formToInput(f: HostFormFields): HostFormResult { identityFile: identityFile || undefined, password: password || undefined, tags, - notes: notes || undefined + notes: notes || undefined, + monitoring: f.monitoring, + monitorPort } }; } diff --git a/crates/omnyssh-gui/ui/src/lib/screens/serverCard.test.ts b/crates/omnyssh-gui/ui/src/lib/screens/serverCard.test.ts index e51d5b4..40489a2 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/serverCard.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/screens/serverCard.test.ts @@ -5,7 +5,11 @@ import type { HostServices } from '$lib/stores/services'; import { deriveCard, metricStatus, QUICK_ACTIONS, filterHosts } from './serverCard'; function host(name = 'web-1'): HostDto { - return { name, hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false }; + return { name, hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false, monitoring: 'ssh' }; +} + +function tcpHost(name = 'fw-1'): HostDto { + return { ...host(name), monitoring: 'tcpPort', monitorPort: 8443 }; } function metrics(partial: Partial): MetricsDto { @@ -198,3 +202,37 @@ describe('quick actions', () => { expect(get(sessions).map((s) => s.kind)).toEqual(['terminal', 'sftp']); }); }); + +describe('deriveCard — reachability hosts', () => { + it('shows the probe result instead of metric tiles', () => { + const card = deriveCard(tcpHost(), { kind: 'connected' }, undefined, undefined); + expect(card.reachability).toBe('reachable'); + expect(card.metricRows).toEqual([]); + expect(card.overall).toBe('ok'); + expect(card.offline).toBe(false); + }); + + it('reports a failed probe as unreachable and offline', () => { + const card = deriveCard(tcpHost(), { kind: 'failed', message: 'refused' }, undefined, undefined); + expect(card.reachability).toBe('unreachable'); + expect(card.offline).toBe(true); + expect(card.overall).toBe('off'); + }); + + it('is still checking before the first probe answers', () => { + expect(deriveCard(tcpHost(), { kind: 'connecting' }, undefined, undefined).reachability).toBe('checking'); + expect(deriveCard(tcpHost(), undefined, undefined, undefined).reachability).toBe('checking'); + }); + + it('never claims metrics an ssh host would have reported', () => { + const card = deriveCard(tcpHost(), { kind: 'connected' }, metrics({ cpuPercent: 90 }), undefined); + expect(card.metricRows).toEqual([]); + expect(card.uptime).toBeUndefined(); + }); + + it('leaves an ssh host on the metric path', () => { + const card = deriveCard(host(), { kind: 'connected' }, metrics({ cpuPercent: 10 }), undefined); + expect(card.reachability).toBeUndefined(); + expect(card.metricRows).toHaveLength(3); + }); +}); diff --git a/crates/omnyssh-gui/ui/src/lib/screens/serverCard.ts b/crates/omnyssh-gui/ui/src/lib/screens/serverCard.ts index 9960c82..9cd1c04 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/serverCard.ts +++ b/crates/omnyssh-gui/ui/src/lib/screens/serverCard.ts @@ -30,12 +30,17 @@ export type MetricRow = { label: string; percent: number | null; status: Status export type CardService = { kind: ServiceKindDto; name: string; detail: string }; +/** Reachability of a `tcpPort` host; `undefined` for an SSH-monitored one. */ +type Reachability = 'reachable' | 'unreachable' | 'checking'; + export interface ServerCard { host: HostDto; /** Header dot: connected health, `off` when failed, else neutral. */ overall: Status; /** Down/unprobed with no live metrics — the card shows an offline state. */ offline: boolean; + /** Set only for a reachability host, which has no metrics to show. */ + reachability?: Reachability; metricRows: MetricRow[]; uptime?: string; osInfo?: string; @@ -92,6 +97,21 @@ export function deriveCard( m: MetricsDto | undefined, svc: HostServices | undefined ): ServerCard { + // A reachability host is probed by a TCP connect and never reports metrics, so + // the tiles would be a fiction — the card shows the probe result instead. + if (host.monitoring !== 'ssh') { + const kind = status?.kind; + return { + host, + overall: kind === 'connected' ? 'ok' : kind === 'failed' ? 'off' : 'unknown', + offline: kind === 'failed', + reachability: kind === 'connected' ? 'reachable' : kind === 'failed' ? 'unreachable' : 'checking', + metricRows: [], + topProcesses: [], + detectedServices: [] + }; + } + const metricRows: MetricRow[] = [ metricRow('CPU', m?.cpuPercent), metricRow('RAM', m?.ramPercent), diff --git a/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.test.ts b/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.test.ts index 561ab42..cc9c4f9 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.test.ts @@ -3,7 +3,7 @@ import type { ConnectionStatusDto, HostDto, MetricsDto } from '$lib/bindings'; import { deriveHostSummary } from './hostSummary'; function host(name: string): HostDto { - return { name, hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false }; + return { name, hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false, monitoring: 'ssh' }; } function metrics(partial: Partial): MetricsDto { @@ -118,3 +118,27 @@ describe('deriveHostSummary — partition invariant', () => { } }); }); + +describe('deriveHostSummary — reachability hosts', () => { + it('does not count a stale sample against a host that no longer reports metrics', () => { + const fw: HostDto = { ...host('fw'), monitoring: 'tcpPort' }; + const summary = deriveHostSummary( + [fw], + new Map([['fw', { kind: 'connected' } as ConnectionStatusDto]]), + // Left over from before the host was switched to a port check. + new Map([['fw', metrics({ cpuPercent: 92 })]]) + ); + + expect(summary).toEqual({ total: 1, online: 1, alert: 0, offline: 0 }); + }); + + it('still counts a breaching ssh host as an alert', () => { + const summary = deriveHostSummary( + [host('web')], + new Map([['web', { kind: 'connected' } as ConnectionStatusDto]]), + new Map([['web', metrics({ cpuPercent: 92 })]]) + ); + + expect(summary.alert).toBe(1); + }); +}); diff --git a/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.ts b/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.ts index 7fbafe9..dc73cfd 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.ts @@ -39,7 +39,10 @@ export function deriveHostSummary( let offline = 0; for (const host of hostList) { if (statusMap.get(host.name)?.kind === 'connected') { - if (isBreaching(metricMap.get(host.name))) alert++; + // A reachability host never refreshes metrics; a sample left over from + // before the switch would pin it in `alert` until the app restarts. + const sample = host.monitoring === 'ssh' ? metricMap.get(host.name) : undefined; + if (isBreaching(sample)) alert++; else online++; } else { offline++; diff --git a/crates/omnyssh-gui/ui/src/lib/stores/palette.test.ts b/crates/omnyssh-gui/ui/src/lib/stores/palette.test.ts index a143609..6fda2d5 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/palette.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/palette.test.ts @@ -13,6 +13,7 @@ function host(name: string, extra: Partial = {}): HostDto { tags: [], source: 'manual', hasKey: false, + monitoring: 'ssh', ...extra }; } From 93b4ca1c425b5fe7274ed6b5c3c5b5774f7e5873 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 23:32:48 +0400 Subject: [PATCH 13/13] docs(changelog): note the TCP monitoring mode --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc476f1..0618b34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,10 +9,13 @@ Versions follow [Semantic Versioning](https://semver.org/). ## Unreleased +### Features +- **Watch a host by a port check instead of an SSH login.** Firewalls, switches and other appliances answer SSH but have no shell to read `top` or `free` from, so monitoring could only ever fail on them — and log in again every cycle to find that out. A host can now be set to **TCP port check**: OmnySSH opens a connection to the port and closes it, with no login and no commands. Its card shows reachable / unreachable in place of the metric tiles, rather than tiles for numbers nobody collected. Set it in the host form — `tcp` (the host's SSH port) or `tcp:PORT` in the terminal app, a dropdown in the desktop app. Existing hosts are untouched and stay on SSH monitoring. ICMP is not offered yet: unprivileged ping is unavailable on the Linux packaging most people use. + ### Bug Fixes - **Hosts split across `Include` files are imported.** A relative pattern — `Include conf.d/*.conf`, the form nearly every split-config guide prints — was looked for in whatever directory the app happened to be launched from, which for the desktop app started from Finder or the application menu is `/`. Nothing matched, so every host defined in `~/.ssh/conf.d` was missing and had to be added by hand. Those patterns now resolve against `~/.ssh`, the way `ssh` itself resolves them. Four things in the same code path were fixed alongside it: full glob patterns work (`?`, `[abc]`, a wildcard in a directory name, and more than one `*` in a file name), several pathnames on one `Include` line are all read instead of none, a quoted path keeps its spaces, and an `Include` written inside a `Host` block no longer swallows that host's remaining settings. An `Include` that matches nothing is written to the log rather than passing in silence. - **CPU no longer reads ~91–100 % on an idle server whose system language isn't English.** Monitoring reads the idle percentage from `top` and shows the rest as used. On a server set to a language with a comma decimal separator, `top` prints `99,1 id`, and the parser split that line on commas — reading the idle value as `1` and reporting 99 % used on a machine doing nothing. The same servers could show RAM and Disk as N/A, because `free` and `df` translate the labels being looked for, and the process list could come back empty. The monitoring commands now run in a fixed locale, and the CPU parsers understand a decimal comma on their own for hosts where that cannot be set. Process names keep the server's own character set. -- **A host that answers SSH but not shell commands is no longer re-logged-in every 30 seconds.** Network appliances — firewalls, switches — authenticate fine but cannot run `top` or `free`, and a round of failed metric commands was treated as a dead connection. The retry delay was reset on every successful login, so it never grew past its first step: one login every 30 seconds, indefinitely, and as often as every 10 seconds in the desktop app, whose refresh timer also cut the retry delay short. The delay now escalates properly and is left alone by the refresh timer. Separately, a device that ignores SSH keepalives was being disconnected after 30 seconds even while its commands still worked; connections are now held open, with liveness still bounded by the keepalive limit. +- **A host that answers SSH but not shell commands is no longer re-logged-in every 30 seconds.** Network appliances — firewalls, switches — authenticate fine but cannot run `top` or `free`, and a round of failed metric commands was treated as a dead connection. The retry delay was reset on every successful login, so it never grew past its first step: one login every 30 seconds, indefinitely, and as often as every 10 seconds in the desktop app, whose refresh timer also cut the retry delay short. The delay now escalates properly and is left alone by the refresh timer — the trade-off being that a manual refresh no longer cuts a retry delay short, so a host that has been unreachable for a while retries on its own schedule (at most five minutes). Separately, a device that ignores SSH keepalives was being disconnected after 30 seconds even while its commands still worked; connections are now held open, with liveness still bounded by the keepalive limit. - **`ProxyJump` hosts now connect through their bastion.** A host with `ProxyJump` in `~/.ssh/config` was parsed but never routed: the terminal refused to open it, while metrics, SFTP, snippets and key setup quietly dialled the target address direct — which for an internal host meant every connection failed or, worse, landed somewhere else on that address. Every native SSH path now walks the jump chain the way `ssh -J` does, connecting and authenticating each bastion in turn and tunnelling the next hop over it. - The jump alias is resolved against your host list, so `ProxyJump public-proxy` picks up that entry's `HostName`, `User`, `Port` and `IdentityFile`. An alias that matches no entry is used as a literal hostname. - Multi-hop values (`ProxyJump first,second`), inline `user@host:port` overrides, IPv6 literals, bastions that are themselves behind a bastion, and the `ProxyJump none` opt-out all behave as OpenSSH does — including its precedence rule that only the first hop of a list contributes bastions of its own. Chains that loop, or run past ten hops, are reported instead of hanging.