Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ 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 <host>` 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

### Added
Expand Down
3 changes: 2 additions & 1 deletion src/commands/download.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
103 changes: 103 additions & 0 deletions src/commands/error_format.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// 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)");
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)"),
"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");
}
}
13 changes: 11 additions & 2 deletions src/commands/interactive/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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)
);
}
}
}
Expand Down Expand Up @@ -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)
);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/commands/ping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
39 changes: 31 additions & 8 deletions src/jump/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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::*;
Expand Down Expand Up @@ -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"));
}
}
Loading