fix: show full anyhow error chain on interactive connect failure

[inureyes]

Summary

Interactive-mode connection failures used to print only the outermost anyhow context via eprintln!("{}", e), hiding the failing hop; this adds a shared helper that renders the full anyhow context chain and wires it into both interactive execution sites plus the other user-facing sites in src/commands/ that had the same problem. Because that fix makes the full chain visible for the first time, it also exposed several pre-existing message-construction defects that would have rendered as duplicated or backwards text once the whole chain was shown; those are fixed here too, since a chain that reads backwards or repeats itself defeats the purpose of showing it.

What changed

• Added src/commands/error_format.rs with format_connection_error(&anyhow::Error) -> String, which renders the full context chain via the alternate Display form ({:#}), matching the format already used by src/executor/parallel.rs (left untouched) so exec-mode and interactive-mode output stay visually consistent.
• Registered the new module in src/commands/mod.rs.
• src/commands/interactive/execution.rs: both the PTY-path and traditional-path connection failure prints (lines ~64 and ~133) now use the helper instead of bare {} Display.
• src/commands/download.rs: the directory-download failure print now uses the helper instead of e.to_string(), since download_dir_from_node goes through the same jump-host chain and was losing the same context.
• src/commands/ping.rs: the error-chain print now reuses the shared helper instead of its own inline format!("{e:#}"), for consistency; output and per-line indentation are unchanged since the helper does the same formatting.
• Audited the remaining eprintln!/println! sites in src/commands/ and left them alone: exec.rs's port-forwarding warning wraps a leaf anyhow!() with no added context, the ReadlineError prints in interactive/single_node.rs and interactive/multiplex.rs are not anyhow errors at all, and interactive/multiplex.rs:212 prints an anyhow::Error with {} whose source is a bare leaf russh::Error with no .context() layer added, so {} and {:#} render identically there and switching it would be a no-op.
• Added unit tests in error_format.rs covering a nested .context() chain (all layers present in output), two different failure hops producing distinguishable messages that each contain their own root cause, and a single-layer error formatting correctly.

Fixed defects the chain rendering would have made visible

• src/jump/chain.rs: the intermediate jump-hop .with_context() interpolated the jump host twice, producing Failed to connect to jump host bastion (hop 2): bastion. The closure is now a small, unit-tested intermediate_jump_hop_context helper that interpolates the host once, producing Failed to connect to jump host bastion (hop 2). The sibling first-jump and destination contexts were checked and did not have the same defect.
• src/ssh/client/connection.rs and src/ssh/client/file_transfer.rs: both built their anyhow chain backwards via anyhow!(friendly_message).context(e). Since .context()'s argument becomes the outer layer, 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 is outermost and e is the cause.
• Correcting that ordering exposed a second defect in the same messages: because {:#} 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 underlying russh error that the variant's own Display already carried. connect_error_message now returns Option<String> and yields None for variants whose Display already says everything, so those get no extra layer at all; 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 own Display already includes.
• Context messages no longer end with a period, which previously rendered as the awkward sequence .: in the middle of a line.
• Since SshError is now shown directly rather than behind a wrapping layer, its message text became user-visible, so the occured misspelling and the Ssh capitalization are fixed in SshError and SftpError in src/ssh/tokio_client/error.rs.
• Added regression tests covering the corrected ordering, the variants that must not get a context layer, the inner key error appearing exactly once, and the absence of trailing periods.

Test plan

• cargo check --lib --tests
• cargo test --lib (1275 passed, 0 failed)
• cargo test --lib commands::error_format (3 passed)
• cargo test --lib commands::interactive (18 passed)
• cargo test --lib jump::chain (17 passed, including 2 new)
• cargo test --lib ssh::client::connection (8 passed, including 4 new)
• cargo test --lib ssh::client::file_transfer (4 passed, new module)
• cargo clippy --lib --tests -- -D warnings (clean)
• cargo fmt --all -- --check (clean)
• Verified src/executor/parallel.rs has no diff (its format!("{e:#}") behavior is unchanged, per the issue's acceptance criteria)
• Manually traced the four failure scenarios from the issue (first-jump, intermediate-hop, destination channel-open, destination auth) through src/jump/chain.rs and src/jump/chain/tunnel.rs context wrapping to confirm each now surfaces its own distinct innermost cause through the helper

Note on an unrelated pre-existing flake: commands::interactive::utils::tests::test_expand_path_with_tilde fails nondeterministically under the full parallel cargo test --lib because several tests call EnvGuard::set("HOME", ...), mutating process-global env while that test reads dirs::home_dir() twice. It passes in isolation on main and on this branch, and is untouched by this PR. No test added here mutates HOME.

Closes #238

[inureyes]

Implementation Review Summary

Intent

Interactive-mode connection failures must print the full anyhow context chain, via a shared unit-tested helper wired into both interactive execution sites, without changing src/executor/parallel.rs or -vv tracing behavior.

Findings Addressed

• Missing CHANGELOG.md entry for a user-facing fix (HIGH). This repository adds an ## [Unreleased] / ### Fixed bullet referencing the issue number for every user-facing fix (precedents: fix: restore enhanced keyboard modes after PTY disconnect #234 in 41642d9, feat: preserve outer TUI enhanced keyboard state across PTY sessions #236 in 63d2b31, and fix: restore terminal mouse tracking state on PTY session disconnect #189 in 127246b, which created the [Unreleased] heading in exactly this post-release situation). Because the release commit rolls that section into the version heading mechanically, the omission would have dropped this fix from the next release notes. Fixed in 1f25fab.

Remaining Items

• src/commands/ping.rs:94 still inlines format!("{e:#}"), which is now byte-identical to the body of format_connection_error (LOW). Behavior is already correct (full chain), so this is a cosmetic reuse opportunity in the same directory the PR audited, deliberately left out of scope.
• src/commands/interactive/multiplex.rs:212 prints an anyhow::Error with {} and is not listed in the PR description's audit narrative (LOW). Re-audited independently: NodeSession::send_command is self.channel.data(..).await?, a bare ? conversion of a leaf russh::Error with no .context() layer, so {} and {:#} are equivalent there and leaving it alone is the right call. Only the PR description's enumeration is incomplete, not the code.
• The distinguishes_different_failure_hops test covers two of the four failure classes named in the issue's first acceptance criterion, and builds errors by hand rather than exercising src/jump/chain.rs (LOW). The criterion is satisfied by the distinct context strings already present at chain.rs:272 (first jump host), chain.rs:304 (hop N), chain.rs:329 (destination through chain), tunnel.rs:206 (direct-tcpip channel open), and tunnel.rs:256 (destination auth), each of which now reaches the terminal.

Independent audit of src/commands/

All 92 print!/println!/eprintln! sites were enumerated. No genuinely-wrapped anyhow error print was missed, and no changed site should have been left alone:

• exec.rs:208: correctly left alone. ForwardingManager::stop_forwarding has exactly one error path, the leaf anyhow!("Forwarding session {id} not found"), with no .context() layer.
• interactive/single_node.rs:203 and interactive/multiplex.rs:313: correctly left alone, both are rustyline::error::ReadlineError, not anyhow errors.
• ping.rs:92-101: already renders the full chain with format!("{e:#}").
• download.rs and upload.rs non-directory paths: failures are reported through ExecutionResult::print_summary, which already uses format!("{e:#}") at src/executor/result_types.rs:103 and :143.
• download.rs:141 was correctly changed: download_dir_from_node reaches SshClient::download_dir_with_jump_hosts, which adds .with_context() at src/ssh/client/file_transfer.rs:586, :606, and :613 on top of the jump-chain contexts, so e.to_string() was discarding a real chain.

Verification

• All stated requirements implemented. Both interactive sites (execution.rs:65 PTY path, execution.rs:138 traditional path) call the shared helper.
• No placeholder/mock code remaining.
• Integrated into project code flow. format_connection_error has three live call sites and is not orphaned.
• Project conventions followed. Apache header, module docs, inline format args, pub fn matching sibling commands/ modules; cargo fmt --all --check clean.
• Existing shared modules reused. The helper matches the format!("{e:#}") form already used across the codebase rather than inventing a new rendering.
• No unintended structural changes. One new file plus three single-expression edits, no renames or moves.
• src/executor/parallel.rs behavior unchanged. git diff main...HEAD -- src/executor/parallel.rs is empty and line 517 still reads format!("{e:#}").
• -vv tracing verbosity unchanged. No tracing:: call, subscriber, or log configuration is touched.
• Unit test asserts every context layer. includes_all_context_layers asserts all four layers of the nested chain individually.
• Tests pass. cargo check --lib --tests clean, cargo test --lib commands::error_format 3 passed, cargo test --lib commands::interactive 18 passed, cargo clippy --lib --tests -- -D warnings clean.

[inureyes]

Security and performance review

Verdict: no CRITICAL or HIGH findings, nothing auto-fixed, no commits added. The change is sound and I recommend merging. Two follow-up items below, both pre-existing rather than introduced here.

I traced every context layer reachable from the three new call sites through interactive/connection.rs, jump/chain.rs, jump/chain/tunnel.rs, jump/chain/auth.rs, jump/parser/, ssh/auth.rs, ssh/client/connection.rs, ssh/client/file_transfer.rs, ssh/tokio_client/, and the upstream russh / russh::keys / ssh_key Display impls.

No credential material can reach stderr through {:#}

• Passwords and passphrases: every failure context on these paths is either a static string ("Failed to read password", "Failed to read passphrase", "Password prompt timed out") or interpolates only host and username (jump/chain/auth.rs:293-307). Secret values appear only in rpassword prompt text, never in an error value. Password wraps secrecy::SecretString and has a hand-written redacting Debug (security/password.rs:96-102). BSSH_PASSWORD is never echoed (security/password.rs:140-148).
• Key material: AuthMethod does hold key_data: Zeroizing<String>, but it is never Debug-formatted in production code. The only {:?} on it is a test panic (ssh/auth.rs:832), and jump/chain/auth.rs:271-285 deliberately hand-matches variants into fixed strings inside a debug! rather than formatting the enum. I checked russh::keys::Error variant by variant (russh-0.62.1/src/keys/mod.rs:89-186): bad-passphrase and decryption failures render as fixed strings such as "The key is encrypted" and "The key is corrupt", UnsupportedKeyType prints only the algorithm name and excludes key_type_raw, and the IO variant is transparent over a std::fs::File::open error that std does not attach a path to. ssh_key::Error is likewise fixed-string only.
• SSH agent socket: only the literal env var name SSH_AUTH_SOCK ever appears. All four decision points use presence-only checks (ssh/auth.rs:359,382,772 via var_os(..).is_some()), the agent error is discarded at tokio_client/authentication.rs:309, and the one place the socket path is formatted is a debug! (jump/chain/auth.rs:120).
• Auth tokens and Backend.AI credentials: none exist on these paths.
• Host key verification failures collapse to the payload-free Error::ServerCheckFailed (tokio_client/connection.rs:527-565), so a changed host key does not surface the offending key or the known_hosts line.

What genuinely does become visible (LOW)

Worth knowing, though all of it is local-context detail shown to the user in their own terminal rather than disclosure to a third party:

• Private key file paths, including $HOME-derived defaults and canonicalized (symlink-resolved) paths: jump/chain/auth.rs:168,218,357-362,375-381,385-391, ssh/auth.rs:178,182,553,646-649,665,672, ssh/client/connection.rs:57, interactive/connection.rs:158. Paths only, never contents.
• Jump hop identities beyond what was typed. This is the largest behavioral delta. When -J is absent, the hop chain comes from the bssh config file's jump_host (interactive, via config/resolver.rs:282-326) or from ~/.ssh/config ProxyJump (executor/connection_manager.rs:258-265), and every hop's user@host:port now prints at jump/chain.rs:274,306,429 and jump/chain/tunnel.rs:64,72,100,127,143,157. Previously the outermost layer showed the target host only.
• The local OS login name via whoami::username() when the user omits user@ (jump/parser/host.rs:62-66, printed at jump/chain/tunnel.rs:157-159).
• The count of keys loaded in the local SSH agent (jump/chain/auth.rs:445-450).
• The canonicalized local download destination, so -d ./out surfaces as an absolute path (shared/validation.rs:141-149, printed at ssh/client/file_transfer.rs:589,608,615).

Only worth acting on if bssh output is routinely captured into a shared CI log or artifact. Note that shared/validation.rs:411 sanitize_error_message looks like the natural hook but is the wrong tool: it redacts hostnames, IPs, and usernames, which are exactly the diagnostic details this PR sets out to surface, and it would mangle these particular strings since many contain " to " and " from ". It is also currently dead code, exported but never called.

MEDIUM, pre-existing: unbounded remote-controlled text reaches the terminal

crates/bssh-russh-sftp/src/client/error.rs:12 renders #[error("{}: {}", .0.status_code, .0.error_message)], and error_message is deserialized straight off the SFTP wire (crates/bssh-russh-sftp/src/protocol/status.rs:49). It goes through Display rather than Debug, so newlines, ANSI escapes, and other control bytes pass through verbatim. ssh/client/file_transfer.rs:607-616 wraps it under "Failed to download directory from ...", so e.to_string() previously hid it and the new site prints it.

Since the download.rs site is a println! inside the per-node loop that also emits Successfully downloaded directory and a trailing summary, a hostile node can inject newlines plus crafted text and forge bssh's own status lines, including lines that appear to describe other nodes. For a multi-node fleet tool that is an output-integrity concern rather than a disclosure one.

To be clear about attribution: this is not introduced by this PR. commands/ping.rs:94 and executor/result_types.rs:103,143 already print the full untruncated chain by iterating error_chain.lines(), and result_types.rs:143 is the single-file download display fed by the same SFTP error source. So the PR is a genuine parity change. It does add two more sites, and it also creates the first natural central hook for fixing this: collapsing control characters inside format_connection_error, either by mapping \n, \r, and other control bytes after rendering or by sanitizing each layer while iterating error.chain(), would cover the new sites and could then be adopted by the existing ones. Reasonable as a follow-up issue rather than a blocker.

LOW, pre-existing, now rendered backwards

ssh/client/connection.rs:147 Err(anyhow!(error_msg).context(e)) and ssh/client/file_transfer.rs:678 Err(anyhow!(detailed).context(e)) have their nesting inverted: the friendly message becomes the inner cause and the raw error the outer context. Under {:#} these now render raw error first and friendly message second, sometimes near-duplicated, for example Ssh error occured: X: SSH connection error: X. Swapping the argument order would clean up the output. Separately, jump/chain.rs:306-309 interpolates jump_host twice into one string, so that line prints user@host:port redundantly.

One more note on {:#} semantics that works in the PR's favor: it walks anyhow::Chain, which includes source() links of foreign errors, not just .context() layers. That is why Error::IoError, whose own Display is the bare "I/O error" (ssh/tokio_client/error.rs:42-43), now also yields the inner Connection refused (os error 111). That is exactly the detail issue #238 was about.

Performance: no regression

format!("{error:#}") is one String allocation on the failure path only, once per node, bounded by a chain depth of roughly four to six layers. download.rs is allocation-neutral because e.to_string() already allocated one String. execution.rs adds one String where {} previously formatted directly into the stderr lock, which is immaterial next to the network timeout that preceded it. Nothing changes on the success path and no lock is held across the formatting.

Verification

cargo check --lib --tests clean, cargo test --lib commands::error_format 3 of 3 pass, cargo clippy --lib --tests -- -D warnings clean. Working tree untouched.