fix: wire up the -4/-6 address family flags and AddressFamily

[inureyes]

Summary

-4/--ipv4 and -6/--ipv6 were declared, shown in --help, and documented in the man page, but nothing ever read them: bssh -6 against a dual-stack host could still connect over IPv4. The ssh_config AddressFamily keyword was dead the same way, parsed and merged during host-config resolution and then discarded. This wires both up end to end, implementing the six decisions signed off on the issue.

Design

Both sources share one representation, AddressFamily { Any, V4, V6 } in src/ssh/tokio_client/address_family.rs. It is resolved once per dispatch path with OpenSSH precedence (command line flag over config keyword over the any default) and carried on SshConnectionConfig, the struct every connection path already threads, so exec, interactive, ping, the jump chain, and port forwarding all inherit it without a separate parameter. connect_with_config_inner filters the resolved candidate list before attempting any connection; Any returns the resolver's list untouched, which keeps the unflagged path byte-for-byte identical to before. The SFTP paths never carried a connection config (they relied on establish_connection substituting the default), so they thread the Copy AddressFamily value alone and rebuild that same default with the family applied.

What changed, per decision point

• Decision 1, forwarding listeners. -6 moves the implicit -L/-D listen address from 127.0.0.1 to ::1, and the *:port wildcard form from 0.0.0.0 to ::. -4 and the no-flag default keep the IPv4 loopback. An explicit bind address in the spec always wins. ForwardingSpec::parse_local/parse_dynamic and the new parse_bind_spec_with_family take the preference; Cli::parse_port_forwards now takes it too. This is a user-visible default change and has its own changelog entry.
• Decision 2, forwarding targets. Client::open_direct_tcpip_channel_with_family filters the candidate list; open_direct_tcpip_channel delegates to it with Any so nothing else changes. ForwardingConfig carries the family to the -L forwarder and the SOCKS5 -D handler. SOCKS4 carries a literal IPv4 destination by protocol definition and is passed through unfiltered, with a comment saying so. The man page states this is a best-effort hint because the remote sshd performs the connect.
• Decision 3, jump hops past the first. Unchanged and documented: those connections ride an existing channel with no local TCP connect, so the remote sshd resolves. The first hop goes through JumpChain::connect_to_first_jump, which already uses connect_with_ssh_config, so it is covered automatically.
• Decision 4, handler address in tunnel.rs. Both to_socket_addrs().next() sites are replaced by a shared resolve_handler_address that prefers a candidate of the forced family, falls back to the first resolved address when none matches (no panic), and logs the fallback at debug level.
• Decision 5, failure mode. New Error::NoAddressForFamily { host, family } renders as no IPv6 address found for <host> (and the IPv4 equivalent), replacing the generic could not resolve to any addresses, and returns through the same non-zero exit path as other connection failures. No fallback to the other family.
• Decision 6, AddressFamily values. Case-insensitive any|inet|inet6; an unrecognized value warns through tracing and falls back to any rather than rejecting a config file OpenSSH would accept.

Two adjacent gaps were closed so the flags are not silently ignored on some subcommands: ping now receives the resolved SshConnectionConfig (which also makes it honor ServerAliveInterval/Compression like exec already did), and the port-forwarding carrier connection in commands/exec.rs switched from Client::connect to Client::connect_with_ssh_config. src/cli/pdsh.rs still hardcodes ipv4: false, ipv6: false; pdsh has no equivalent flag, so that is left as-is.

Tests added

tests/address_family_test.rs (9 tests): ssh_config keyword honored per host, command line flag over config keyword, unrecognized value falling back to any, -6 changing the -L/-D listen default, the no-flag IPv4 default, an explicit bind address overriding the flag, a dual-stack loopback test that binds both families on the same port and asserts which listener actually accepts the connection under -4 and under -6 (skipped with a message when IPv6 loopback is unavailable), the unforced path still connecting, and the hard-failure message.

Unit tests: 12 in address_family.rs covering the filter (mixed list yields only IPv4 under -4, only IPv6 under -6, and the original list unchanged and in order with neither), emptying, first_match, case-insensitive config parsing, precedence, and the bind-address defaults; 5 in connection_tests.rs covering the empty-after-filter error path on the real connect path for both families, the default staying unconstrained, builder chaining, and the unforced path never reporting a family mismatch; 4 in forwarding/spec.rs and 1 in forwarding/mod.rs for the listener defaults; 3 in jump/chain/tunnel.rs for the handler address preference and fallback.

Docs

docs/man/bssh.1: the -4/-6 entries are rewritten, AddressFamily is documented under a new Connection Options subsection, and a new ADDRESS FAMILY SELECTION section spells out what the constraint covers (direct connects, first jump hop, -L/-D listener), what it only hints at (forwarding targets, SOCKS4 exemption), what it does not cover (later jump hops, -R listener, bssh-server), and the failure behavior. ARCHITECTURE.md gains an "Address Family Preference" section describing the representation, threading, scope table, and failure mode. CHANGELOG.md gains two entries under Unreleased/Fixed, one of which calls out the forwarding listener default change and the migration (name the bind address explicitly).

Test plan

• cargo clippy --lib --bins --tests -- -D warnings clean
• cargo test --test address_family_test (9 passed)
• cargo test --lib ssh:: (348 passed), cargo test --lib forwarding:: (24 passed), cargo test --lib jump:: (48 passed), cargo test --lib cli:: (24 passed)
• cargo test --test ssh_keepalive_test (34 passed), cargo test --test pdsh_compat_test (35 passed)
• cargo test --doc forwarding (2 passed)

Closes #246

[inureyes]

Implementation Review Summary

Intent

Make the previously-parsed-but-ignored -4/-6 flags and the ssh_config AddressFamily keyword actually constrain which resolved address bssh connects to, with OpenSSH precedence, across every dispatch route.

Verification

• All stated requirements implemented (all six signed-off decisions traced to code)
• No placeholder/mock code remaining
• Integrated into project code flow (exec, interactive, ping, upload, download, port forwarding, first jump hop)
• Project conventions followed (get_address_family mirrors get_compression/get_proxy_jump; builder style matches with_compression; thiserror variant on the existing Error enum)
• Existing modules reused where applicable (no duplicated resolution or filtering logic; SshConnectionConfig carries the preference rather than a new parameter)
• No unintended structural changes
• Tests pass (targeted selectors, see below)

The no-flag invariant holds

ToSocketAddrsWithHostname::to_socket_addrs already returns Vec<SocketAddr>, and AddressFamily::filter early-returns that Vec untouched when the family is not forced. There is no re-collection, sort, or dedup on any path. Checked every application site:

Site	Any behavior
connect_with_config_inner	identity; the is_forced() guard also wraps the empty-list error, so an empty resolver result still yields the original AddressInvalid("could not resolve to any addresses")
open_direct_tcpip_channel	delegates with Any, identity
parse_bind_spec	delegates with Any; Any.loopback() is 127.0.0.1, Any.unspecified() is 0.0.0.0
ForwardingSpec::parse_remote	hardcodes Any
ForwardingConfig::default()	Any
resolve_handler_address (tunnel.rs)	skips both branches, falls through to .next(), identical to the old code

Locked in by any_filter_returns_the_original_list_unchanged_and_in_order and test_unforced_family_does_not_produce_the_family_error.

Wiring completeness

Every dispatch route reaches a connect call that carries the preference. Client::connect is now used in production code only from SshClient::connect_for_file_transfer, which is itself unreachable (see below). Everything else goes through connect_with_ssh_config.

src/cli/pdsh.rs hardcoding ipv4: false, ipv6: false is coherent: pdsh has no equivalent flag, and AddressFamily::resolve(false, false, cfg) still falls through to the ssh_config keyword rather than forcing Any.

Findings

MEDIUM. The man page and ARCHITECTURE.md give a factually wrong reason for excluding jump hops past the first.

Both say those connections "ride an existing channel with no local TCP connect ... the remote server resolves the address and bssh cannot influence which family it picks." The first half is right; the second is not. src/jump/chain/tunnel.rs:99 and :236 call open_direct_tcpip_channel, and that function resolves the target locally and sends target.ip().to_string(), a literal IP, in the channel-open request (src/ssh/tokio_client/channel_manager.rs:214-222). bssh, not the remote sshd, picks the family for hop 2 onward and for the destination behind a chain. So bssh -6 -J bastion target cmd still reaches target over whichever family the local resolver happens to order first, and it is the same mechanism decision 2 already approved for -L targets.

Decision 3 scoped the behavior out and I am not asking to reverse it. Hard-failing an IPv4-only intermediate hop under -6 is a real reason to keep it out. But the shipped rationale is wrong, and a user reading "bssh cannot influence which family it picks" will never file the follow-up. Suggest rewording both places to say bssh names a locally-resolved literal address there and constraining it is deliberately deferred, plus a follow-up issue. Consequence worth noting in the same breath: decision 4 makes resolve_handler_address pick a -6-matching address for the known_hosts context on exactly the hops whose channel target is still unfiltered, so under -6 on a dual-stack hop the recorded address and the actual channel target can be different families.

LOW. Error chain now repeats host:port on the jump-host resolution failure path. resolve_handler_address returns No addresses resolved for: {host}:{port} and both call sites wrap it with Failed to resolve jump host address: {host}:{port} / Failed to resolve destination address: {host}:{port}. Under the {:#} rendering this repo adopted in #238, the empty-resolution case now prints the host and port twice on one line, which is the pattern the CHANGELOG entry "Stop repeating the same wording twice in a rendered connection-error chain" set out to remove. Before this PR each case produced one message.

LOW. parse_bind_spec is now reachable only from its own unit test. All three production call sites moved to parse_bind_spec_with_family. Keeping it as a documented public back-compat shim is defensible; flagging so the state is known.

LOW. AddressFamily from ssh_config is looked up with the hostname "*" in cluster and -H mode. hostname_for_ssh_config is Some(..) only when cli.is_ssh_mode(), so bssh -H node1.example.com uptime picks up AddressFamily only from a Host * block, not from Host node1.example.com. This is identical to the existing behavior for Compression (#219) and ServerAlive*, so it is convention-consistent and pre-existing rather than introduced here. The -4/-6 flags themselves work in every mode. Worth one sentence in the man page's AddressFamily entry.

LOW, cosmetic. ForwardingType's Display does not bracket IPv6 bind addresses, so under -6 the startup line reads ::1:8080→example.com:80. Pre-existing (reachable before via an explicit [::1] bind), but -6 makes it the default rendering.

Informational, not defects

• ping now inherits the resolved SshConnectionConfig, so ServerAliveInterval, ServerAliveCountMax, and Compression apply to bssh ping where they previously did not. Called out in the PR body as an intentional adjacent fix and consistent with exec.
• The SFTP jump-host path now passes an explicit SshConnectionConfig where it passed None. Verified equivalent: JumpHostChain::new already defaulted to SshConnectionConfig::default() and connect_direct substitutes the same default for None.

file_transfer.rs reachability, verified independently

The claim holds. SshClient::upload_file, download_file, upload_dir, and download_dir (src/ssh/client/file_transfer.rs:59,128,193,260) have no callers anywhere in src/ or tests/. Every .upload_file(/.download_file(/.upload_dir(/.download_dir( call in the repo resolves either to tokio_client::Client (inside file_transfer.rs itself) or to ParallelExecutor, and connection_manager.rs uses only the *_with_jump_hosts variants, which do thread the family. Their private helper connect_for_file_transfer is called only from those four. So no user-facing flow silently ignores the flag there. They are pub on a pub struct in a library crate, so a downstream consumer would get no address family control and no compile error; adding the parameter for symmetry, or marking them deprecated, would close that.

Checks run

cargo check --lib --bins --tests clean. cargo test --lib ssh:: (348), --lib executor:: (53), --lib forwarding:: (24), --lib jump:: (48), --lib cli:: (24), --test address_family_test (9), --test ssh_keepalive_test (34), --test pdsh_compat_test (35), --test jump_host_config_test (38), --test connect_timeout_test (11), --test download_test (4), --doc forwarding (2). All passing.

Remaining items

• Reword the jump-hop rationale in docs/man/bssh.1 and ARCHITECTURE.md (MEDIUM), and decide whether to open a follow-up for constraining the direct-tcpip target on hops past the first
• Optional: collapse the duplicated host:port in the resolve_handler_address error chain (LOW)
• Optional: note the wildcard-only ssh_config lookup in cluster and -H mode in the man page (LOW)

No CRITICAL or HIGH findings, so no automated fixes were applied.

[inureyes]

Review addendum: second-opinion findings

A second reviewer pass surfaced two items. I verified both against the code. Neither changes the verdict (still no CRITICAL or HIGH, nothing auto-fixed), but one of them upgrades a finding I had rated too low and adds a detail I missed.

1. Per-host resolution of the config keyword (upgrading my earlier LOW to MEDIUM)

resolve_address_family (src/app/dispatcher.rs:105-107) is called once per dispatch with the single hostname_for_ssh_config, which is Some(..) only when cli.is_ssh_mode(). Two consequences:

• Cluster and -H mode query the literal "*". bssh -H v6node uptime does not see Host v6node / AddressFamily inet6; only a Host * block applies. Same for ping, upload, and download, which never populate the hostname either.
• The first jump hop inherits the destination's answer. The resolved family rides SshConnectionConfig into JumpHostChain, and JumpHostChain has no ssh_config awareness at all (src/jump/chain.rs:403 connects the first hop with &self.ssh_connection_config). So with Host bastion / AddressFamily inet and Host target / AddressFamily inet6, the bastion connect uses inet6. This part I had missed.

I originally rated this LOW because it is a faithful extension of an established pattern rather than a new defect: build_ssh_connection_config already does exactly this for Compression (hostname.unwrap_or("*"), line 77) and for ServerAliveInterval/ServerAliveCountMax (get_int_option(hostname, ..) with hostname None, lines 59 and 69), and those same values already flow unchanged into every jump hop. Nothing regressed here; one more setting joined a queue that was already mis-scoped.

MEDIUM rather than higher because the subject of #246, the -4/-6 flags, works correctly in every mode, and the issue's acceptance criterion for the keyword is met in SSH-compat mode and via Host *. A correct fix means resolving ssh_config per node and per jump host instead of once per dispatch, which restructures build_ssh_connection_config and JumpHostChain and would take Compression and the keepalive pair with it. That is its own change, not a rider on a bug fix, so I would land it as a follow-up covering all four settings at once rather than widening this PR.

2. Source-breaking public API changes (MEDIUM)

ForwardingSpec::parse_local, parse_dynamic, and parse gained a required AddressFamily argument, as did the four public SshClient::*_with_jump_hosts SFTP helpers. bssh publishes a library target to crates.io, so for any downstream crate this is an immediate compile error, and it is landing under a ### Fixed changelog heading rather than a major bump.

What makes this worth raising is not the semver rule in the abstract, it is that the PR itself already chose the opposite pattern twice: parse_bind_spec was kept as an AddressFamily::Any wrapper next to the new parse_bind_spec_with_family, and open_direct_tcpip_channel was kept as a wrapper next to open_direct_tcpip_channel_with_family. Applying the same shape to ForwardingSpec::parse_local / parse_local_with_family and friends would make the three cases consistent and drop the breakage, at the cost of a few more wrapper functions. The counter-argument is real too: the lib target exists mainly so main.rs can say bssh::, the crate is categorized as command-line-utilities, and there is no in-repo consumer, so a deliberate break may simply be cheaper than carrying wrappers forever. Either answer is defensible; the current state is just inconsistent with itself.

Related and smaller, in the same category: ForwardingConfig gained a public field without #[non_exhaustive], so struct-literal construction breaks, and tokio_client::Error gained a variant, so exhaustive matches break.

Unchanged from the main review

No CRITICAL or HIGH findings. The no-flag invariant holds on every application site, all seven dispatch routes reach a connect call carrying the preference, and the file_transfer.rs unreachability claim is confirmed. Nothing was committed or pushed.

[inureyes]

PR finalization

Addressed the four review-requested corrections and added the tests/docs polish that went with them.

Corrections

• Jump-hop rationale (docs/man/bssh.1, ARCHITECTURE.md, src/jump/chain/tunnel.rs docstring): the old text claimed the remote server resolves hops past the first so bssh cannot influence the family. That was false, bssh resolves the target locally through the same open_direct_tcpip_channel mechanism -L/SOCKS5 -D use; the filter just is not wired to that call yet. Reworded all three places as a scope limitation tracked in issue feat: apply address family preference to jump hops beyond the first #248.
• CHANGELOG.md: moved the source-breaking library signature changes (ForwardingSpec::parse_local/parse_dynamic/parse, the four SshClient::*_with_jump_hosts helpers, ForwardingConfig::address_family, Error::NoAddressForFamily) out of ### Fixed into a new ### Changed entry marked "Breaking, lib API" so the next release picks up the right version bump. The -L/-D listener default change stays under ### Fixed as a distinct CLI behavior change, with its jump-hop wording corrected to match.
• ForwardingType::Display: bracketed IPv6 bind addresses across all three variants (Local, Remote, Dynamic) by building a SocketAddr and reusing its own Display, so -6 renders [::1]:8080→example.com:80 instead of the ambiguous ::1:8080→example.com:80. IPv4 output is byte-for-byte unchanged. Added test_forwarding_type_display_brackets_ipv6.
• Duplicated host:port in the jump error chain: resolve_handler_address restated host:port inside its own error context on top of the identical wording every call site's .with_context() already adds. It now lets the bare resolution error propagate and uses an address-free message for the empty-candidates case, matching the issue fix: interactive mode swallows the anyhow error chain on connection failure, hiding the failing hop #238 convention. Added a regression test that renders the full chain and asserts host:port appears exactly once.

Polish

• Fixed the matching unbracketed IPv6 example in ForwardingSpec's module-level doc comment.
• Tightened the ARCHITECTURE.md scope summary sentence so it no longer implies every non-hard-filter path is at least a "hint" now that the jump-hop row says "not yet filtered."
• Confirmed rustdoc on the touched public items (AddressFamily, Error::NoAddressForFamily, ForwardingConfig::address_family, open_direct_tcpip_channel_with_family, ForwardingSpec::parse*) still matches behavior; no other false-rationale wording found via a repo-wide grep.
• README does not document -4/-6 or AddressFamily today, so left untouched; no Korean or other translated docs exist in this repo.

Validation

• cargo fmt --check, cargo clippy --lib --tests -- -D warnings: clean
• cargo test --lib forwarding:: (25 passed), cargo test --lib jump:: (49 passed), cargo test --test address_family_test (9 passed), cargo test --doc forwarding (2 passed)
• mandoc -Tlint docs/man/bssh.1: no new warnings
• CI, MSRV, and CLA checks on this PR: all passing

Left alone (out of scope, as instructed)

• No back-compat wrappers added for the changed signatures.
• Address family filtering for jump hops 2+ (issue feat: apply address family preference to jump hops beyond the first #248) and per-node SSH config resolution (issue fix: resolve per-host ssh_config connection settings per node instead of once per dispatch #249) left deferred.
• parse_bind_spec untouched.
• No version number changes.
• The 3 pre-existing tests/integration_test.rs failures from a stale local known_hosts entry were not touched.