fix(attach,doctor): dial the configured ssh_port; name the port on doctor timeouts - #180
Merged
Conversation
…ctor timeouts Two follow-ups from the review sweep. 1. remoteattach dropped the SSH port. #174 threaded SSHPort through the TUI's create-session forms, but every argv builder here ignored it, so ccmux created a session on a custom-port host and then dialed 22 to attach — stranding the user with a session they couldn't reach. The same drop hit the Network screen's shell action and both discovered-peer and configured-host attach paths. Port is now a required parameter on SSH/SSHInteractive/Mosh/RunArgv rather than a separate *Port variant. The old shape had exactly that split — SSHInteractivePort existed while callers reached for the port-blind SSHInteractive — which is how the bug survived. Making it required means a new call site has to decide. 0 and 22 produce argv byte-identical to before. mosh needs different syntax and that trap gets its own test: mosh's -p is the UDP port range for the mosh session, NOT the ssh port, so passing it the way ssh takes it would request a UDP bind on 2222 while still connecting to SSH on 22. The port travels via --ssh. 2. doctor's timeout branch didn't name the port while refused/OK both did — so the one case where 'did we probe the wrong port?' is the live question was the case that stayed silent. Extracted the branch copy into probeResultLine so every message is testable without a live host, and pinned the rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The two follow-ups left over from the review sweep, both with fail-before-verified tests.
1. The attach path ignored
ssh_port#174 threaded
SSHPortthrough the TUI's create-session forms, but every argv builder ininternal/remoteattachdropped it. The result: ccmux created a session on a custom-port host correctly, then dialed port 22 to attach to it — leaving the user stranded with a session they couldn't reach. The same drop hit the Network screen's shell action and both the discovered-peer and configured-host attach paths.Fail-before output is exactly the reported symptom:
Port is now a required parameter on
SSH/SSHInteractive/Mosh/RunArgvrather than a separate*Portvariant. That's deliberate: the old shape had precisely that split —SSHInteractivePortexisted while call sites reached for the port-blindSSHInteractive— which is how the bug survived. Making it required means a new call site has to make a decision. Passing 0 or 22 produces argv byte-identical to before (pinned by a test).The mosh trap gets its own test. mosh's own
-pis the UDP port range for the mosh session, not the SSH port — passing it the way ssh takes it would request a UDP bind on 2222 while still connecting to SSH on 22. The port has to travel via--ssh=ssh -p N. The test asserts both that--sshis present and that no bare-pappears.2. doctor's timeout branch hid the port
The refused and OK branches both named the probed port; timeout didn't. So on a custom-port host, the one case where "did we probe the wrong port?" is the live question was the case that stayed silent about it.
Extracted the branch copy into
probeResultLineso every message is testable without a live host, and pinned the rule: any branch whose cause could be a wrong port must name it — whileProbeNoNetworkdeliberately does not, since name resolution failed before anything was dialed.Verification
TestSSH_CustomPortAddsFlag,TestDefaultPorts_ArgvUnchanged,TestMosh_CustomPortUsesSSHFlagNotDashP,TestRunArgv_PortPerBinary,TestRemoteNewSessionAttach_DialsConfiguredPort,TestRemoteNewSessionAttach_DefaultPortUnchanged, and threeprobeResultLinetests.go test ./...green; e2e doctor tests green (they assert on these strings); linux + windows cross-compile clean.🤖 Generated with Claude Code