From d612a0aa9cc3319d82c85394aa7c2ecb1790e14f Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Jul 2026 11:48:49 +0900 Subject: [PATCH 1/4] fix: show full anyhow error chain on interactive connect failure Interactive-mode connection failures printed only the outermost anyhow context via eprintln!("{}", e), hiding the actual failing hop. A user hitting a destination sshd that was not yet accepting connections saw only "Failed to establish jump host connection", with the real cause (a refused direct-tcpip channel open) invisible. Add format_connection_error() in the new src/commands/error_format.rs module, which renders an anyhow::Error's full context chain via the alternate Display form ({:#}), matching the format already used by src/executor/parallel.rs so exec-mode and interactive-mode output stay consistent. src/executor/parallel.rs itself is untouched. Wire the helper into both interactive execution sites (PTY path and traditional path in src/commands/interactive/execution.rs) and into the directory-download error path in src/commands/download.rs, which shares the same jump-host connection stack and previously discarded context via e.to_string(). Other eprintln!/println! sites in src/commands/ were audited and left alone: they either print leaf errors with no wrapped context (exec.rs port-forwarding warning) or a foreign error type with no anyhow chain (ReadlineError in interactive/single_node.rs and interactive/multiplex.rs). Add unit tests in error_format.rs asserting that all layers of a nested .context() chain appear in the formatted output, that different failure hops (first-jump vs destination-auth) produce distinguishable messages containing their own root cause, and that a single-layer error still formats correctly. Refs #238 --- src/commands/download.rs | 3 +- src/commands/error_format.rs | 104 ++++++++++++++++++++++++++ src/commands/interactive/execution.rs | 13 +++- src/commands/mod.rs | 1 + 4 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 src/commands/error_format.rs diff --git a/src/commands/download.rs b/src/commands/download.rs index e2fda5fc..bcf64539 100644 --- a/src/commands/download.rs +++ b/src/commands/download.rs @@ -17,6 +17,7 @@ use owo_colors::OwoColorize; use std::path::Path; use tokio::fs; +use crate::commands::error_format::format_connection_error; use crate::executor::{self, ParallelExecutor}; use crate::ssh::SshClient; use crate::ui::OutputFormatter; @@ -140,7 +141,7 @@ pub async fn download_file( " {} {} {}", "●".red(), "Failed to download directory:".red(), - e.to_string().dimmed() + format_connection_error(&e).dimmed() ); total_failed += 1; } diff --git a/src/commands/error_format.rs b/src/commands/error_format.rs new file mode 100644 index 00000000..8a24af39 --- /dev/null +++ b/src/commands/error_format.rs @@ -0,0 +1,104 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Shared error formatting for user-facing connection failure messages. +//! +//! Connection errors (interactive mode, directory downloads, etc.) are +//! typically built from several layers of `anyhow::Context`, one per hop or +//! stage: for example "Failed to establish jump host connection" wrapping +//! "Failed to connect to jump host (hop N)" wrapping "Failed to open +//! direct-tcpip channel" wrapping the underlying I/O or SSH error. +//! +//! anyhow's default `Display` implementation (`{}`) only prints the +//! outermost message, which hides exactly the detail a user needs to +//! understand *which* hop failed and why. This module centralizes the fix: +//! render the full context chain instead of just the outer layer. + +/// Render the full context chain of a connection-related [`anyhow::Error`]. +/// +/// This uses anyhow's alternate `Display` form (`{:#}`), which walks the +/// entire error chain and joins each layer with `": "`, e.g. +/// `"outer context: middle context: root cause"`. This mirrors the format +/// already used by the parallel exec path (`src/executor/parallel.rs`), so +/// interactive-mode and exec-mode terminal output stay visually consistent. +pub fn format_connection_error(error: &anyhow::Error) -> String { + format!("{error:#}") +} + +#[cfg(test)] +mod tests { + use super::*; + use anyhow::anyhow; + + #[test] + fn includes_all_context_layers() { + // Build a nested error mirroring the real jump-chain wrapping: + // destination auth failure -> destination channel-open failure -> + // intermediate hop failure -> outer "establish jump host connection". + let root = anyhow!("authentication failed for user 'alice' on '10.0.0.5:22'"); + let with_channel = + root.context("Failed to open direct-tcpip channel to destination 10.0.0.5:22"); + let with_hop = + with_channel.context("Failed to connect to jump host 'bastion' (hop 2): bastion"); + let with_outer = + with_hop.context("Failed to establish jump host connection to 10.0.0.5:22"); + + let formatted = format_connection_error(&with_outer); + + assert!( + formatted.contains("Failed to establish jump host connection to 10.0.0.5:22"), + "missing outer context layer in: {formatted}" + ); + assert!( + formatted.contains("Failed to connect to jump host 'bastion' (hop 2): bastion"), + "missing intermediate hop layer in: {formatted}" + ); + assert!( + formatted.contains("Failed to open direct-tcpip channel to destination 10.0.0.5:22"), + "missing channel-open layer in: {formatted}" + ); + assert!( + formatted.contains("authentication failed for user 'alice' on '10.0.0.5:22'"), + "missing innermost root cause in: {formatted}" + ); + } + + #[test] + fn distinguishes_different_failure_hops() { + // A first-jump-hop failure and a destination-auth failure should + // produce visibly different formatted output (both must contain + // their own innermost cause), so the user can tell them apart. + let first_jump_root = anyhow!("connection refused"); + let first_jump = first_jump_root.context("Failed to connect to first jump host: bastion1"); + + let dest_auth_root = anyhow!("permission denied (publickey)"); + let dest_auth = dest_auth_root + .context("Failed to authenticate to destination '10.0.0.5:22' as user 'alice'"); + + let first_jump_msg = format_connection_error(&first_jump); + let dest_auth_msg = format_connection_error(&dest_auth); + + assert_ne!(first_jump_msg, dest_auth_msg); + assert!(first_jump_msg.contains("connection refused")); + assert!(dest_auth_msg.contains("permission denied (publickey)")); + } + + #[test] + fn single_layer_error_still_formats() { + // Guard against the helper depending on multi-layer chains: a bare + // error with no added context should just render its own message. + let error = anyhow!("simple failure"); + assert_eq!(format_connection_error(&error), "simple failure"); + } +} diff --git a/src/commands/interactive/execution.rs b/src/commands/interactive/execution.rs index 5646d3c7..eb555b73 100644 --- a/src/commands/interactive/execution.rs +++ b/src/commands/interactive/execution.rs @@ -18,6 +18,7 @@ use anyhow::Result; use owo_colors::OwoColorize; use std::sync::Arc; +use crate::commands::error_format::format_connection_error; use crate::pty::PtyManager; use super::super::interactive_signal::{ @@ -61,7 +62,11 @@ impl InteractiveCommand { connected_nodes.push(node); } Err(e) => { - eprintln!("✗ Failed to connect to {}: {}", node.to_string().red(), e); + eprintln!( + "✗ Failed to connect to {}: {}", + node.to_string().red(), + format_connection_error(&e) + ); } } } @@ -130,7 +135,11 @@ impl InteractiveCommand { sessions.push(session); } Err(e) => { - eprintln!("✗ Failed to connect to {}: {}", node.to_string().red(), e); + eprintln!( + "✗ Failed to connect to {}: {}", + node.to_string().red(), + format_connection_error(&e) + ); } } } diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 52442301..a2518112 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -13,6 +13,7 @@ // limitations under the License. pub mod download; +pub mod error_format; pub mod exec; pub mod interactive; pub mod interactive_signal; From 1f25faba00c81b91d7ec86a7cddc2c7f74c89ea0 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Jul 2026 11:58:11 +0900 Subject: [PATCH 2/4] docs: add changelog entry for interactive error chain fix This PR is a user-facing bug fix but shipped without a CHANGELOG bullet, and since the release commit mechanically rolls the `## [Unreleased]` section into the new version heading, the fix would have been silently absent from the next release notes. The repository convention for a user-facing fix is a `## [Unreleased]` / `### Fixed` bullet referencing the issue number rather than the PR number (see 41642d9 for #234 and 127246b for #189, which likewise opened a fresh Unreleased section immediately after a release), so this adds that section back on top of the just-released v2.3.1 heading with a single bullet describing the interactive connection error chain fix. Documentation only, no code or behavior change. Refs #238 --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6418ac0a..3b695f88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ All notable changes to bssh will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed +- **Show the full error context chain when an interactive connection fails** (#238). Interactive mode printed only anyhow's outermost context, so a jump-host failure surfaced as `Failed to establish jump host connection to ` with no indication of which hop failed or why, leaving first-jump authentication failures, destination TCP refusals, and destination authentication failures indistinguishable. Both interactive execution paths (PTY and traditional) and the directory-download failure message now render the whole chain through a shared `format_connection_error` helper that uses anyhow's alternate `Display` form, matching the format the parallel exec path already used. Tracing verbosity and exec-mode output are unchanged. + ## [2.3.1] - 2026-07-29 ### Added From 67233d2e65bea86bd08fd58621bd5da80bef91b2 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Jul 2026 12:22:12 +0900 Subject: [PATCH 3/4] fix: correct duplicated and backwards error context in connect paths The #238 chain-rendering fix makes the full anyhow context chain visible for the first time, which surfaced three pre-existing defects in the messages being wrapped: they would now render as duplicated or backwards text instead of just being silently truncated by anyhow's default Display. src/jump/chain.rs's intermediate jump-hop `.with_context()` interpolated the jump host twice ("Failed to connect to jump host bastion (hop 2): bastion"). The closure is now a small testable `intermediate_jump_hop_context` helper that interpolates the host once. src/ssh/client/connection.rs and src/ssh/client/file_transfer.rs both built their anyhow chain backwards via `anyhow!(friendly_message).context(e)`; `.context()`'s argument becomes the outer layer, so the raw SSH error rendered first and the friendly message rendered underneath it as a near-duplicate cause. Both now use `anyhow::Error::new(e).context(friendly_message)` so the friendly message is outermost and `e` is the cause, and their catch-all match arms no longer interpolate `e`'s own Display text into the friendly message (which would otherwise duplicate the cause verbatim once the ordering is corrected). src/commands/ping.rs's error-chain print now reuses the shared `format_connection_error` helper instead of its own inline `format!("{e:#}")`; output is unchanged since the helper does the same formatting. src/commands/error_format.rs's test fixture data is updated to match the corrected jump-hop message instead of the old duplicated form. src/executor/parallel.rs is untouched. Validation: - cargo check --lib --tests - cargo test --lib jump::chain (17 passed, including 2 new) - cargo test --lib ssh::client::connection (6 passed, including 2 new) - cargo test --lib ssh::client::file_transfer (2 passed, new module) - cargo test --lib commands::error_format (3 passed) - cargo clippy --lib --tests -- -D warnings (clean) - cargo fmt --all -- --check (clean) Refs #238 --- CHANGELOG.md | 1 + src/commands/error_format.rs | 5 +- src/commands/ping.rs | 3 +- src/jump/chain.rs | 39 +++++++--- src/ssh/client/connection.rs | 122 ++++++++++++++++++++++++-------- src/ssh/client/file_transfer.rs | 50 ++++++++++++- 6 files changed, 175 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b695f88..ac36c749 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Show the full error context chain when an interactive connection fails** (#238). Interactive mode printed only anyhow's outermost context, so a jump-host failure surfaced as `Failed to establish jump host connection to ` with no indication of which hop failed or why, leaving first-jump authentication failures, destination TCP refusals, and destination authentication failures indistinguishable. Both interactive execution paths (PTY and traditional) and the directory-download failure message now render the whole chain through a shared `format_connection_error` helper that uses anyhow's alternate `Display` form, matching the format the parallel exec path already used. Tracing verbosity and exec-mode output are unchanged. +- **Fix three pre-existing error-message defects that the #238 chain rendering would otherwise have made user-visible**. `src/jump/chain.rs`'s intermediate jump-hop context interpolated the jump host twice, printing `Failed to connect to jump host bastion (hop 2): bastion` instead of `Failed to connect to jump host bastion (hop 2)`. The direct-connect error in `src/ssh/client/connection.rs` and the file-transfer connect error in `src/ssh/client/file_transfer.rs` both built their anyhow chain backwards as `anyhow!(friendly_message).context(e)`, which makes `.context()`'s argument the *outer* layer, so the raw SSH error rendered first and the friendly message rendered underneath it as a near-duplicate cause; both now build `anyhow::Error::new(e).context(friendly_message)` so the friendly message renders first and `e` renders as the cause. `src/commands/ping.rs`'s error-chain print now reuses the shared `format_connection_error` helper instead of its own inline `format!("{e:#}")` for consistency; its per-line indentation is unchanged. ## [2.3.1] - 2026-07-29 diff --git a/src/commands/error_format.rs b/src/commands/error_format.rs index 8a24af39..c5a773d6 100644 --- a/src/commands/error_format.rs +++ b/src/commands/error_format.rs @@ -49,8 +49,7 @@ mod tests { let root = anyhow!("authentication failed for user 'alice' on '10.0.0.5:22'"); let with_channel = root.context("Failed to open direct-tcpip channel to destination 10.0.0.5:22"); - let with_hop = - with_channel.context("Failed to connect to jump host 'bastion' (hop 2): bastion"); + let with_hop = with_channel.context("Failed to connect to jump host bastion (hop 2)"); let with_outer = with_hop.context("Failed to establish jump host connection to 10.0.0.5:22"); @@ -61,7 +60,7 @@ mod tests { "missing outer context layer in: {formatted}" ); assert!( - formatted.contains("Failed to connect to jump host 'bastion' (hop 2): bastion"), + formatted.contains("Failed to connect to jump host bastion (hop 2)"), "missing intermediate hop layer in: {formatted}" ); assert!( diff --git a/src/commands/ping.rs b/src/commands/ping.rs index 7bf06e96..5f713021 100644 --- a/src/commands/ping.rs +++ b/src/commands/ping.rs @@ -17,6 +17,7 @@ use owo_colors::OwoColorize; use std::path::Path; use std::sync::Arc; +use crate::commands::error_format::format_connection_error; use crate::executor::ParallelExecutor; use crate::node::Node; use crate::security::Password; @@ -91,7 +92,7 @@ pub async fn ping_nodes( ); if let Err(e) = &result.result { // Display the full error chain for better debugging - let error_chain = format!("{e:#}"); + let error_chain = format_connection_error(e); // Split by newlines and indent each line for (i, line) in error_chain.lines().enumerate() { if i == 0 { diff --git a/src/jump/chain.rs b/src/jump/chain.rs index 809540a3..7f7e4c69 100644 --- a/src/jump/chain.rs +++ b/src/jump/chain.rs @@ -301,14 +301,7 @@ impl JumpHostChain { &self.ssh_connection_config, ) .await - .with_context(|| { - format!( - "Failed to connect to jump host {} (hop {}): {}", - jump_host, - i + 2, - jump_host - ) - })?; + .with_context(|| intermediate_jump_hop_context(jump_host, i + 2))?; debug!("Connected through jump host: {}", jump_host); } @@ -452,6 +445,16 @@ impl Drop for JumpHostChain { } } +/// Build the `.with_context()` message for a failed intermediate jump hop. +/// +/// `hop` is the 1-based position of `jump_host` within the full chain (i.e. +/// already offset past the first jump host). The host is interpolated only +/// once; see issue #238 for the readability defect this replaced, where the +/// host name was duplicated as both the subject and a trailing echo. +fn intermediate_jump_hop_context(jump_host: &JumpHost, hop: usize) -> String { + format!("Failed to connect to jump host {jump_host} (hop {hop})") +} + #[cfg(test)] mod tests { use super::*; @@ -487,4 +490,24 @@ mod tests { assert_eq!(chain.max_retries, 5); assert_eq!(chain.base_retry_delay, 2000); } + + #[test] + fn test_intermediate_jump_hop_context_does_not_repeat_host_name() { + let jump_host = JumpHost::new("bastion".to_string(), None, None); + let message = intermediate_jump_hop_context(&jump_host, 2); + + assert_eq!(message, "Failed to connect to jump host bastion (hop 2)"); + // The host name must appear exactly once; issue #238's readability + // review found it interpolated twice (subject + trailing echo). + assert_eq!(message.matches("bastion").count(), 1); + } + + #[test] + fn test_intermediate_jump_hop_context_reports_correct_hop_number() { + let jump_host = JumpHost::new("relay.example.com".to_string(), None, None); + let message = intermediate_jump_hop_context(&jump_host, 3); + + assert!(message.contains("hop 3")); + assert!(message.contains("relay.example.com")); + } } diff --git a/src/ssh/client/connection.rs b/src/ssh/client/connection.rs index b147cd3b..aee33e89 100644 --- a/src/ssh/client/connection.rs +++ b/src/ssh/client/connection.rs @@ -28,6 +28,48 @@ use std::time::Duration; // - Balances user patience with reliability on poor networks const SSH_CONNECT_TIMEOUT_SECS: u64 = 30; +/// Build the friendly, outer-context message for a failed direct SSH +/// connection attempt. +/// +/// The returned message is meant to be used as `anyhow::Error::new(e).context(message)` +/// so it renders as the outer layer and `e` renders as the cause. It +/// deliberately does not repeat `e`'s own `Display` text so `{:#}` output +/// does not show the same wording twice; see issue #238. +fn connect_error_message(e: &crate::ssh::tokio_client::Error) -> String { + match e { + crate::ssh::tokio_client::Error::KeyAuthFailed => { + "Authentication failed. The private key was rejected by the server.".to_string() + } + crate::ssh::tokio_client::Error::PasswordWrong => { + "Password authentication failed.".to_string() + } + crate::ssh::tokio_client::Error::ServerCheckFailed => { + "Host key verification failed. The server's host key was not recognized or has changed." + .to_string() + } + crate::ssh::tokio_client::Error::KeyInvalid(key_err) => { + format!( + "Failed to load SSH key: {key_err}. Please check the key file format and passphrase." + ) + } + crate::ssh::tokio_client::Error::AgentConnectionFailed => { + "Failed to connect to SSH agent. Please ensure SSH_AUTH_SOCK is set and the agent is running." + .to_string() + } + crate::ssh::tokio_client::Error::AgentNoIdentities => { + "SSH agent has no identities. Please add your key to the agent using 'ssh-add'." + .to_string() + } + crate::ssh::tokio_client::Error::AgentAuthenticationFailed => { + "SSH agent authentication failed.".to_string() + } + crate::ssh::tokio_client::Error::SshError(ssh_err) => { + format!("SSH connection error: {ssh_err}") + } + _ => "Failed to connect".to_string(), + } +} + impl SshClient { /// Determine the authentication method based on provided parameters. /// @@ -114,37 +156,12 @@ impl SshClient { { Ok(Ok(client)) => Ok(client), Ok(Err(e)) => { - // Specific error from the SSH connection attempt - let error_msg = match &e { - crate::ssh::tokio_client::Error::KeyAuthFailed => { - "Authentication failed. The private key was rejected by the server.".to_string() - } - crate::ssh::tokio_client::Error::PasswordWrong => { - "Password authentication failed.".to_string() - } - crate::ssh::tokio_client::Error::ServerCheckFailed => { - "Host key verification failed. The server's host key was not recognized or has changed.".to_string() - } - crate::ssh::tokio_client::Error::KeyInvalid(key_err) => { - format!("Failed to load SSH key: {key_err}. Please check the key file format and passphrase.") - } - crate::ssh::tokio_client::Error::AgentConnectionFailed => { - "Failed to connect to SSH agent. Please ensure SSH_AUTH_SOCK is set and the agent is running.".to_string() - } - crate::ssh::tokio_client::Error::AgentNoIdentities => { - "SSH agent has no identities. Please add your key to the agent using 'ssh-add'.".to_string() - } - crate::ssh::tokio_client::Error::AgentAuthenticationFailed => { - "SSH agent authentication failed.".to_string() - } - crate::ssh::tokio_client::Error::SshError(ssh_err) => { - format!("SSH connection error: {ssh_err}") - } - _ => { - format!("Failed to connect: {e}") - } - }; - Err(anyhow::anyhow!(error_msg).context(e)) + // Specific error from the SSH connection attempt. + // The friendly message is the outer context and `e` is the + // cause, so `{:#}` renders ": " + // instead of the reverse; see issue #238. + let error_msg = connect_error_message(&e); + Err(anyhow::Error::new(e).context(error_msg)) } Err(_) => Err(anyhow::anyhow!( "Connection timeout after {} seconds. \ @@ -460,4 +477,47 @@ mod tests { _ => panic!("Expected PrivateKeyFile auth method"), } } + + #[test] + fn test_connect_error_ordering_puts_friendly_message_first() { + // Regression test for issue #238's readability defect: the friendly + // message must be the OUTER context so `{:#}` renders it first, + // followed by the underlying cause, instead of the reverse. + let e = crate::ssh::tokio_client::Error::PasswordWrong; + let error_msg = connect_error_message(&e); + let err = anyhow::Error::new(e).context(error_msg); + + let rendered = format!("{err:#}"); + assert!( + rendered.starts_with("Password authentication failed."), + "expected friendly message first, got: {rendered}" + ); + // The underlying cause (thiserror's own Display text) must still be + // present, appearing after the friendly message rather than before it. + let friendly_end = rendered.find("Password authentication failed.").unwrap() + + "Password authentication failed.".len(); + assert!( + rendered[friendly_end..].contains("Password authentication failed"), + "expected the cause to appear after the friendly message, got: {rendered}" + ); + } + + #[test] + fn test_connect_error_message_catch_all_does_not_duplicate_cause() { + // The catch-all arm used to interpolate `{e}` directly into the + // friendly message; once nesting is corrected that would print the + // cause's text twice. It must not repeat the variant's own text. + let e = crate::ssh::tokio_client::Error::CommandDidntExit; + let error_msg = connect_error_message(&e); + assert_eq!(error_msg, "Failed to connect"); + + let cause_text = e.to_string(); + let err = anyhow::Error::new(e).context(error_msg); + let rendered = format!("{err:#}"); + assert_eq!( + rendered.matches(cause_text.as_str()).count(), + 1, + "cause text should appear exactly once, got: {rendered}" + ); + } } diff --git a/src/ssh/client/file_transfer.rs b/src/ssh/client/file_transfer.rs index c7441732..d152cee9 100644 --- a/src/ssh/client/file_transfer.rs +++ b/src/ssh/client/file_transfer.rs @@ -675,7 +675,7 @@ impl SshClient { Ok(Err(e)) => { let context = format!("SSH connection to {}:{}", self.host, self.port); let detailed = format_ssh_error(&context, &e); - Err(anyhow::anyhow!(detailed).context(e)) + Err(anyhow::Error::new(e).context(detailed)) } Err(_) => Err(anyhow::anyhow!( "Connection timeout after {timeout_secs} seconds. Host may be unreachable or SSH service not running." @@ -755,6 +755,52 @@ fn format_ssh_error(context: &str, e: &crate::ssh::tokio_client::Error) -> Strin crate::ssh::tokio_client::Error::AgentAuthenticationFailed => { format!("{context} failed: SSH agent authentication rejected") } - _ => format!("{context} failed: {e}"), + _ => format!("{context} failed"), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_connect_for_file_transfer_error_ordering_puts_friendly_message_first() { + // Regression test for issue #238's readability defect: `detailed` + // must be the OUTER context so `{:#}` renders it first, followed by + // the underlying cause, instead of the reverse. + let e = crate::ssh::tokio_client::Error::PasswordWrong; + let context = "SSH connection to host:22".to_string(); + let detailed = format_ssh_error(&context, &e); + let err = anyhow::Error::new(e).context(detailed.clone()); + + let rendered = format!("{err:#}"); + assert!( + rendered.starts_with(&detailed), + "expected friendly message first, got: {rendered}" + ); + assert!( + rendered[detailed.len()..].contains("Password authentication failed"), + "expected the cause to appear after the friendly message, got: {rendered}" + ); + } + + #[test] + fn test_format_ssh_error_catch_all_does_not_duplicate_cause() { + // The catch-all arm used to interpolate `{e}` directly into the + // detailed message; once nesting is corrected that would print the + // cause's text twice. It must not repeat the variant's own text. + let e = crate::ssh::tokio_client::Error::CommandDidntExit; + let context = "SSH connection to host:22".to_string(); + let detailed = format_ssh_error(&context, &e); + assert_eq!(detailed, "SSH connection to host:22 failed"); + + let cause_text = e.to_string(); + let err = anyhow::Error::new(e).context(detailed); + let rendered = format!("{err:#}"); + assert_eq!( + rendered.matches(cause_text.as_str()).count(), + 1, + "cause text should appear exactly once, got: {rendered}" + ); } } From 6d4fbbbc8d8d6d85445fed0b8a5429081d335cb0 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Jul 2026 12:29:42 +0900 Subject: [PATCH 4/4] fix: stop repeating the cause in connection error context layers Correcting the inverted anyhow nesting exposed a second defect in the same messages. Because anyhow's `{:#}` form joins layers with ": ", a context message that restates its own cause prints the same wording twice on one line. The direct-connect path returned a context message for every error variant, so `PasswordWrong` rendered as "Password authentication failed.: Password authentication failed" and `SshError` interpolated the russh error that the variant's own Display already carried. `connect_error_message` now returns `Option` and yields `None` for variants whose Display already says everything, so those get no extra layer. The messages it does return add remediation guidance without echoing the cause. `KeyInvalid`'s context in both the direct-connect and file-transfer paths no longer re-interpolates the inner key error, which the variant's Display already includes. Context messages also no longer end with a period, which rendered as the sequence ".: " mid-line. Since `SshError` is now shown directly rather than behind a wrapping layer, its message text is user-visible, so the `occured` misspelling and the `Ssh` capitalization are fixed there and in `SftpError`. Adds regression tests covering the omitted-context variants, the inner key error appearing exactly once, and the absence of trailing periods. --- CHANGELOG.md | 1 + src/ssh/client/connection.rs | 156 +++++++++++++++++++++----------- src/ssh/client/file_transfer.rs | 57 +++++++++++- src/ssh/tokio_client/error.rs | 4 +- 4 files changed, 158 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac36c749..6f2cd932 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Show the full error context chain when an interactive connection fails** (#238). Interactive mode printed only anyhow's outermost context, so a jump-host failure surfaced as `Failed to establish jump host connection to ` with no indication of which hop failed or why, leaving first-jump authentication failures, destination TCP refusals, and destination authentication failures indistinguishable. Both interactive execution paths (PTY and traditional) and the directory-download failure message now render the whole chain through a shared `format_connection_error` helper that uses anyhow's alternate `Display` form, matching the format the parallel exec path already used. Tracing verbosity and exec-mode output are unchanged. - **Fix three pre-existing error-message defects that the #238 chain rendering would otherwise have made user-visible**. `src/jump/chain.rs`'s intermediate jump-hop context interpolated the jump host twice, printing `Failed to connect to jump host bastion (hop 2): bastion` instead of `Failed to connect to jump host bastion (hop 2)`. The direct-connect error in `src/ssh/client/connection.rs` and the file-transfer connect error in `src/ssh/client/file_transfer.rs` both built their anyhow chain backwards as `anyhow!(friendly_message).context(e)`, which makes `.context()`'s argument the *outer* layer, so the raw SSH error rendered first and the friendly message rendered underneath it as a near-duplicate cause; both now build `anyhow::Error::new(e).context(friendly_message)` so the friendly message renders first and `e` renders as the cause. `src/commands/ping.rs`'s error-chain print now reuses the shared `format_connection_error` helper instead of its own inline `format!("{e:#}")` for consistency; its per-line indentation is unchanged. +- **Stop repeating the same wording twice in a rendered connection-error chain**. Because anyhow's `{:#}` form joins layers with `": "`, a context message that restates its own cause prints the same text twice on one line. The direct-connect path returned a context layer for every error variant, so `PasswordWrong` rendered as `Password authentication failed.: Password authentication failed` and `SshError` interpolated the underlying russh error that the variant's own `Display` already carried; it now returns a context layer only for variants whose `Display` does not already say everything, and those messages add remediation guidance without echoing the cause. `KeyInvalid`'s context in both the direct-connect and file-transfer paths no longer re-interpolates the inner key error. Context messages also no longer end with a period, which previously rendered as the sequence `.: ` mid-line. Fixed the `occured` misspelling and the `Ssh` capitalization in the `SshError` and `SftpError` messages, which are now shown to the user directly rather than behind a wrapping context layer. ## [2.3.1] - 2026-07-29 diff --git a/src/ssh/client/connection.rs b/src/ssh/client/connection.rs index aee33e89..2cbe1a47 100644 --- a/src/ssh/client/connection.rs +++ b/src/ssh/client/connection.rs @@ -31,42 +31,37 @@ const SSH_CONNECT_TIMEOUT_SECS: u64 = 30; /// Build the friendly, outer-context message for a failed direct SSH /// connection attempt. /// -/// The returned message is meant to be used as `anyhow::Error::new(e).context(message)` -/// so it renders as the outer layer and `e` renders as the cause. It -/// deliberately does not repeat `e`'s own `Display` text so `{:#}` output -/// does not show the same wording twice; see issue #238. -fn connect_error_message(e: &crate::ssh::tokio_client::Error) -> String { +/// The result is meant to be used as `anyhow::Error::new(e).context(message)` +/// so the message renders as the outer layer and `e` renders as the cause. +/// Because anyhow's `{:#}` form joins layers with `": "`, a context message +/// that restates the variant's own `Display` text would make the same wording +/// appear twice in one line. So this returns `None` for variants whose +/// `Display` already says everything the context layer would, and the +/// messages it does return add remediation guidance without echoing the +/// cause and carry no trailing period (which would render as `".: "`). +/// See issue #238. +fn connect_error_message(e: &crate::ssh::tokio_client::Error) -> Option { match e { crate::ssh::tokio_client::Error::KeyAuthFailed => { - "Authentication failed. The private key was rejected by the server.".to_string() + Some("The private key was rejected by the server".to_string()) } - crate::ssh::tokio_client::Error::PasswordWrong => { - "Password authentication failed.".to_string() - } - crate::ssh::tokio_client::Error::ServerCheckFailed => { - "Host key verification failed. The server's host key was not recognized or has changed." - .to_string() - } - crate::ssh::tokio_client::Error::KeyInvalid(key_err) => { - format!( - "Failed to load SSH key: {key_err}. Please check the key file format and passphrase." - ) + crate::ssh::tokio_client::Error::ServerCheckFailed => Some( + "Host key verification failed: the server's host key was not recognized or has changed" + .to_string(), + ), + crate::ssh::tokio_client::Error::KeyInvalid(_) => { + Some("Check the key file format and passphrase".to_string()) } crate::ssh::tokio_client::Error::AgentConnectionFailed => { - "Failed to connect to SSH agent. Please ensure SSH_AUTH_SOCK is set and the agent is running." - .to_string() + Some("Ensure SSH_AUTH_SOCK is set and the agent is running".to_string()) } crate::ssh::tokio_client::Error::AgentNoIdentities => { - "SSH agent has no identities. Please add your key to the agent using 'ssh-add'." - .to_string() + Some("Add your key to the agent using 'ssh-add'".to_string()) } - crate::ssh::tokio_client::Error::AgentAuthenticationFailed => { - "SSH agent authentication failed.".to_string() - } - crate::ssh::tokio_client::Error::SshError(ssh_err) => { - format!("SSH connection error: {ssh_err}") - } - _ => "Failed to connect".to_string(), + // `PasswordWrong`, `AgentAuthenticationFailed` and `SshError` already + // render the full story through their own `Display`, and any context + // here would only repeat it. + _ => None, } } @@ -159,9 +154,13 @@ impl SshClient { // Specific error from the SSH connection attempt. // The friendly message is the outer context and `e` is the // cause, so `{:#}` renders ": " - // instead of the reverse; see issue #238. - let error_msg = connect_error_message(&e); - Err(anyhow::Error::new(e).context(error_msg)) + // instead of the reverse. Variants whose own `Display` is + // already sufficient get no extra layer, so the same wording + // is not printed twice; see issue #238. + match connect_error_message(&e) { + Some(error_msg) => Err(anyhow::Error::new(e).context(error_msg)), + None => Err(anyhow::Error::new(e)), + } } Err(_) => Err(anyhow::anyhow!( "Connection timeout after {} seconds. \ @@ -483,41 +482,92 @@ mod tests { // Regression test for issue #238's readability defect: the friendly // message must be the OUTER context so `{:#}` renders it first, // followed by the underlying cause, instead of the reverse. - let e = crate::ssh::tokio_client::Error::PasswordWrong; - let error_msg = connect_error_message(&e); + let e = crate::ssh::tokio_client::Error::KeyAuthFailed; + let cause_text = e.to_string(); + let error_msg = connect_error_message(&e).expect("KeyAuthFailed adds guidance"); let err = anyhow::Error::new(e).context(error_msg); let rendered = format!("{err:#}"); - assert!( - rendered.starts_with("Password authentication failed."), - "expected friendly message first, got: {rendered}" + assert_eq!( + rendered, "The private key was rejected by the server: Key authentication failed", + "expected friendly message first, then the cause" ); - // The underlying cause (thiserror's own Display text) must still be - // present, appearing after the friendly message rather than before it. - let friendly_end = rendered.find("Password authentication failed.").unwrap() - + "Password authentication failed.".len(); + // The underlying cause must appear after the friendly message, not + // before it, and must not be swallowed. + let friendly_end = rendered + .find("The private key was rejected by the server") + .unwrap() + + "The private key was rejected by the server".len(); assert!( - rendered[friendly_end..].contains("Password authentication failed"), + rendered[friendly_end..].contains(&cause_text), "expected the cause to appear after the friendly message, got: {rendered}" ); } #[test] - fn test_connect_error_message_catch_all_does_not_duplicate_cause() { - // The catch-all arm used to interpolate `{e}` directly into the - // friendly message; once nesting is corrected that would print the - // cause's text twice. It must not repeat the variant's own text. - let e = crate::ssh::tokio_client::Error::CommandDidntExit; - let error_msg = connect_error_message(&e); - assert_eq!(error_msg, "Failed to connect"); + fn test_connect_error_message_omits_context_that_would_repeat_cause() { + // Variants whose own `Display` already says everything get no extra + // context layer, so the same wording is not printed twice. Before this + // fix, `PasswordWrong` rendered as + // "Password authentication failed.: Password authentication failed". + for e in [ + crate::ssh::tokio_client::Error::PasswordWrong, + crate::ssh::tokio_client::Error::AgentAuthenticationFailed, + crate::ssh::tokio_client::Error::CommandDidntExit, + ] { + let cause_text = e.to_string(); + assert!( + connect_error_message(&e).is_none(), + "{cause_text} should not get a context layer" + ); + + let rendered = format!("{:#}", anyhow::Error::new(e)); + assert_eq!(rendered, cause_text); + assert_eq!( + rendered.matches(cause_text.as_str()).count(), + 1, + "cause text should appear exactly once, got: {rendered}" + ); + } + } - let cause_text = e.to_string(); - let err = anyhow::Error::new(e).context(error_msg); - let rendered = format!("{err:#}"); + #[test] + fn test_connect_error_message_does_not_duplicate_inner_key_error() { + // `KeyInvalid`'s own `Display` already interpolates the underlying + // key error, so the context layer must not interpolate it again. + let key_err = russh::keys::Error::KeyIsCorrupt; + let inner_text = key_err.to_string(); + let e = crate::ssh::tokio_client::Error::KeyInvalid(key_err); + + let error_msg = connect_error_message(&e).expect("KeyInvalid adds guidance"); + assert!( + !error_msg.contains(&inner_text), + "context must not echo the inner key error, got: {error_msg}" + ); + + let rendered = format!("{:#}", anyhow::Error::new(e).context(error_msg)); assert_eq!( - rendered.matches(cause_text.as_str()).count(), + rendered.matches(inner_text.as_str()).count(), 1, - "cause text should appear exactly once, got: {rendered}" + "inner key error should appear exactly once, got: {rendered}" ); } + + #[test] + fn test_connect_error_messages_have_no_trailing_period() { + // `{:#}` joins layers with ": ", so a trailing period would render as + // the awkward sequence ".: " in the middle of a line. + for e in [ + crate::ssh::tokio_client::Error::KeyAuthFailed, + crate::ssh::tokio_client::Error::ServerCheckFailed, + crate::ssh::tokio_client::Error::AgentConnectionFailed, + crate::ssh::tokio_client::Error::AgentNoIdentities, + ] { + let message = connect_error_message(&e).expect("variant adds guidance"); + assert!( + !message.ends_with('.'), + "context message must not end with a period, got: {message}" + ); + } + } } diff --git a/src/ssh/client/file_transfer.rs b/src/ssh/client/file_transfer.rs index d152cee9..a7cf001a 100644 --- a/src/ssh/client/file_transfer.rs +++ b/src/ssh/client/file_transfer.rs @@ -735,22 +735,25 @@ fn format_ssh_error(context: &str, e: &crate::ssh::tokio_client::Error) -> Strin crate::ssh::tokio_client::Error::KeyAuthFailed => { format!("{context} failed: Authentication rejected with provided SSH key") } - crate::ssh::tokio_client::Error::KeyInvalid(err) => { - format!("{context} failed: Invalid SSH key - {err}") + // The inner key error is not interpolated here: the `KeyInvalid` + // variant's own `Display` already includes it, and this message is the + // outer context layer, so echoing it would print it twice. + crate::ssh::tokio_client::Error::KeyInvalid(_) => { + format!("{context} failed: Invalid SSH key") } crate::ssh::tokio_client::Error::ServerCheckFailed => { format!( - "{context} failed: Host key verification failed. The server's host key is not trusted." + "{context} failed: Host key verification failed, the server's host key is not trusted" ) } crate::ssh::tokio_client::Error::PasswordWrong => { format!("{context} failed: Password authentication rejected") } crate::ssh::tokio_client::Error::AgentConnectionFailed => { - format!("{context} failed: Cannot connect to SSH agent. Ensure SSH_AUTH_SOCK is set.") + format!("{context} failed: Cannot connect to SSH agent, ensure SSH_AUTH_SOCK is set") } crate::ssh::tokio_client::Error::AgentNoIdentities => { - format!("{context} failed: SSH agent has no keys. Use 'ssh-add' to add your key.") + format!("{context} failed: SSH agent has no keys, use 'ssh-add' to add your key") } crate::ssh::tokio_client::Error::AgentAuthenticationFailed => { format!("{context} failed: SSH agent authentication rejected") @@ -784,6 +787,50 @@ mod tests { ); } + #[test] + fn test_format_ssh_error_does_not_duplicate_inner_key_error() { + // `KeyInvalid`'s own `Display` already interpolates the underlying key + // error, so this outer context layer must not interpolate it again. + let key_err = russh::keys::Error::KeyIsCorrupt; + let inner_text = key_err.to_string(); + let e = crate::ssh::tokio_client::Error::KeyInvalid(key_err); + let context = "SSH connection to host:22".to_string(); + + let detailed = format_ssh_error(&context, &e); + assert!( + !detailed.contains(&inner_text), + "context must not echo the inner key error, got: {detailed}" + ); + + let rendered = format!("{:#}", anyhow::Error::new(e).context(detailed)); + assert_eq!( + rendered.matches(inner_text.as_str()).count(), + 1, + "inner key error should appear exactly once, got: {rendered}" + ); + } + + #[test] + fn test_format_ssh_error_messages_have_no_trailing_period() { + // `{:#}` joins layers with ": ", so a trailing period would render as + // the awkward sequence ".: " in the middle of a line. + let context = "SSH connection to host:22".to_string(); + for e in [ + crate::ssh::tokio_client::Error::KeyAuthFailed, + crate::ssh::tokio_client::Error::ServerCheckFailed, + crate::ssh::tokio_client::Error::PasswordWrong, + crate::ssh::tokio_client::Error::AgentConnectionFailed, + crate::ssh::tokio_client::Error::AgentNoIdentities, + crate::ssh::tokio_client::Error::AgentAuthenticationFailed, + ] { + let detailed = format_ssh_error(&context, &e); + assert!( + !detailed.ends_with('.'), + "context message must not end with a period, got: {detailed}" + ); + } + } + #[test] fn test_format_ssh_error_catch_all_does_not_duplicate_cause() { // The catch-all arm used to interpolate `{e}` directly into the diff --git a/src/ssh/tokio_client/error.rs b/src/ssh/tokio_client/error.rs index 087a89e4..a47765b4 100644 --- a/src/ssh/tokio_client/error.rs +++ b/src/ssh/tokio_client/error.rs @@ -23,7 +23,7 @@ pub enum Error { CommandDidntExit, #[error("Server check failed")] ServerCheckFailed, - #[error("Ssh error occured: {0}")] + #[error("SSH error occurred: {0}")] SshError(#[from] russh::Error), #[error("Send error")] SendError(#[from] russh::SendError), @@ -37,7 +37,7 @@ pub enum Error { AgentNoIdentities, #[error("SSH agent authentication failed")] AgentAuthenticationFailed, - #[error("SFTP error occured: {0}")] + #[error("SFTP error occurred: {0}")] SftpError(#[from] russh_sftp::client::error::Error), #[error("I/O error")] IoError(#[from] io::Error),