From fefdd0e640c7698611ee2a5120da1747c62532ce Mon Sep 17 00:00:00 2001 From: oth-body Date: Tue, 15 Sep 2026 16:45:04 -0400 Subject: [PATCH] fix(nip46): make every sign-in error actionable and surface init failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The user's bug report: 'web socket failure, response, no exception, no...' The 'no...' substring maps to the bare 'no response yet' string at nip46.go:371 (renamed the polling timeout to ErrTimeout with a contextful default). The 'web socket failure' substring never appears in source — it was either a wrapper error or a stale binary. Changes: - Add nip46.ErrTimeout (wraps the bare 'no response yet' string with Amber/approval context), nip46.RelayDialFailure (carries per-URL dial errors instead of discarding them), and nip46.IsRetryable so callers can distinguish transient vs hard failures. - ConnectRelays now returns *RelayDialFailure with per-URL error strings, so users see exactly which relays.txt entry is dead. - warnUnreachableRelays takes the failure map and logs the cause inline (DNS, TLS, timeout) instead of just the URL. - TUI's initQR previously swallowed OnInitQR errors via 'return nil' — now surfaces them as qrInitErrorMsg with the actionable message appended below the QR block. checkQRConnection stops polling on non-retryable errors via nip46.IsRetryable instead of looping forever on a hard failure. - Every user-facing error in the package now mentions the failing URL, env var, or an actionable noun (Amber, signer, network, relays.txt). Regression guards in nip46_test.go assert no message leaks the old bare 'no response yet' or 'websocket failure' substrings, and that the bare Reason case is wrapped with context by Error() as a defence-in-depth. Tests: +5 new, full suite green, go vet clean. --- nip46/nip46.go | 211 +++++++++++++++++++++++++++++++++++++------- nip46/nip46_test.go | 196 +++++++++++++++++++++++++++++++++++++--- tui/tui.go | 39 ++++++-- 3 files changed, 401 insertions(+), 45 deletions(-) diff --git a/nip46/nip46.go b/nip46/nip46.go index 5ed1f8b..826ca18 100644 --- a/nip46/nip46.go +++ b/nip46/nip46.go @@ -3,6 +3,7 @@ package nip46 import ( "context" "encoding/json" + "errors" "fmt" "log" "net/url" @@ -235,13 +236,15 @@ func (s *Session) ConnectRelays(ctx context.Context) error { // for any user relay that didn't come up so the user can // clean up a stale relays.txt without having to inspect // every dial result manually. - connected := s.dialRelays(ctx, dialTimeout) + connected, dialFailures := s.dialRelays(ctx, dialTimeout) s.relays = connected if len(connected) == 0 { - return fmt.Errorf("could not connect to any relay (tried: %s)", - strings.Join(s.RelayURLs, ", ")) + return &RelayDialFailure{ + Tried: append([]string(nil), s.RelayURLs...), + Reasons: dialFailures, + } } - warnUnreachableRelays(s.RelayURLs, connected) + warnUnreachableRelays(s.RelayURLs, connected, dialFailures) // Subscribe on every connected relay; merge event channels. // Track how many subscriptions are currently alive so that @@ -284,7 +287,9 @@ func (s *Session) ConnectRelays(ctx context.Context) error { if len(subs) == 0 { cancel() s.Close() - return fmt.Errorf("no relay accepted our subscription (tried: %s)", + return fmt.Errorf("relays connected but every one rejected our subscription (tried: %s); "+ + "this usually means the relay you're hitting is misconfigured or rate-limiting — "+ + "check ~/.config/hoot/relays.txt for stale entries", strings.Join(s.RelayURLs, ", ")) } @@ -317,16 +322,93 @@ func (s *Session) ConnectRelays(ctx context.Context) error { return nil } +// ErrTimeout is returned by WaitForConnection / CheckConnection when the +// polling window elapsed without receiving a signer connect event. It +// wraps the underlying reason so callers can render an actionable +// message ("no signer response yet — did Amber approve the prompt?") +// instead of the previous bare "no response yet" string that users +// couldn't act on. +type ErrTimeout struct { + Reason string // human-readable cause; defaults to "no signer response yet" + Open int // number of subscriptions still open when we timed out +} + +func (e *ErrTimeout) Error() string { + if e.Reason == "" { + return "no signer response yet — make sure Amber has approved the connection prompt and is online" + } + // Guard: if a caller passes a Reason that doesn't include any + // actionable context (Amber, signer, network, etc.), append a + // fallback so the message is never just an opaque bare string + // like "no response yet". Regression for the bug report — the + // original symptom was a bare unhelpful message. + low := strings.ToLower(e.Reason) + actionable := []string{"amber", "signer", "relay", "network", "connection", "subscription", "timeout", "deadline"} + for _, tok := range actionable { + if strings.Contains(low, tok) { + return e.Reason + } + } + return e.Reason + " — check that Amber is online and has approved the connection prompt" +} + +// IsRetryable reports whether an error from a NIP-46 call is +// transient — i.e. the caller should keep polling instead of +// bailing out and showing a fatal error. The TUI uses this to +// distinguish "signer just hasn't replied yet" (retry) from +// "this will never work, surface to the user" (stop and show). +// +// Rules: +// - nil → not retryable (success) +// - ErrTimeout → retryable +// - wrapped timeout / deadline → retryable +// - all-subscriptions-closed err → NOT retryable (network is dead) +// - everything else (validation, → NOT retryable +// proxy refusal, config) (configuration problem) +func IsRetryable(err error) bool { + if err == nil { + return false + } + var et *ErrTimeout + if errors.As(err, &et) { + return et.Open > 0 + } + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { + return true + } + return false +} + // CheckConnection polls for a signer connect event with a short -// timeout. Returns the signer's pubkey on success, or an error if -// nothing arrived yet. Callers should retry until success or a -// hard timeout. This is meant to be called from the TUI's -// OnCheckQR in a polling loop. +// timeout. Returns the signer's pubkey on success, or an *ErrTimeout +// (wrappable via errors.As) if nothing arrived yet — see IsRetryable +// for the recommended polling pattern. This is meant to be called +// from the TUI's OnCheckQR in a polling loop. +// +// The previous version returned a bare "no response yet" string and +// also returned that same string when all subscriptions had died +// (the "all relays dropped" condition). Users couldn't tell the two +// apart, and the message gave them nothing to act on. The current +// version returns ErrTimeout with Open > 0 (retry — Amber may still +// approve) vs a distinct, non-retryable "all relay subscriptions +// closed" error so the TUI can render the right message. func (s *Session) CheckConnection(timeout time.Duration) (string, error) { if s.merged == nil { return "", fmt.Errorf("not connected — call ConnectRelays first") } + // Track how many subscriptions are still open so ErrTimeout can + // distinguish "wait longer" (open > 0) from "network is dead" + // (open == 0). The count is approximate — subscriptions close + // asynchronously — but a single mid-poll snapshot is enough for + // the actionable-message split. + open := 0 + for _, r := range s.relays { + if r.IsConnected() { + open++ + } + } + timer := time.NewTimer(timeout) defer timer.Stop() @@ -337,9 +419,9 @@ func (s *Session) CheckConnection(timeout time.Duration) (string, error) { // s.merged was closed — every relay's subscription // channel has drained. That's only possible if all // our wss:// connections died. Surface this as a - // distinct, actionable error instead of "no events - // yet" so the user knows re-scanning won't help and - // they should check their network. + // distinct, actionable, non-retryable error so the + // TUI shows "all relays dropped — check network" + // instead of looping on "no response yet". return "", fmt.Errorf("all relay subscriptions closed — every relay in the pairing set dropped its connection; check network and try again") } if ev == nil { @@ -368,15 +450,19 @@ func (s *Session) CheckConnection(timeout time.Duration) (string, error) { } case <-timer.C: - return "", fmt.Errorf("no response yet") + return "", &ErrTimeout{ + Reason: "no signer response yet — make sure Amber has approved the connection prompt and is online", + Open: open, + } } } } -func (s *Session) dialRelays(ctx context.Context, perRelayTimeout time.Duration) []*nostr.Relay { +func (s *Session) dialRelays(ctx context.Context, perRelayTimeout time.Duration) (connected []*nostr.Relay, failures map[string]string) { type result struct { relay *nostr.Relay err error + url string } results := make(chan result, len(s.RelayURLs)) var wg sync.WaitGroup @@ -387,35 +473,88 @@ func (s *Session) dialRelays(ctx context.Context, perRelayTimeout time.Duration) dialCtx, cancel := context.WithTimeout(ctx, perRelayTimeout) defer cancel() relay, err := nostr.RelayConnect(dialCtx, u) - results <- result{relay: relay, err: err} + results <- result{relay: relay, err: err, url: u} }(u) } wg.Wait() close(results) - var connected []*nostr.Relay + failures = make(map[string]string) for r := range results { if r.err == nil && r.relay != nil { connected = append(connected, r.relay) + continue + } + // Record the per-URL error so the caller can surface a + // per-relay failure message. nil/empty relays get a generic + // "unknown" reason so we don't emit an empty map. + if r.err != nil { + failures[r.url] = r.err.Error() + } else { + failures[r.url] = "unknown failure (relay URL returned no error and no connection)" } } - return connected + return connected, failures +} + +// RelayDialFailure is returned by WaitForConnection (and ConnectRelays) +// when zero relays could be dialled. It carries per-relay error strings +// so the user sees the actual failure reason (DNS, TLS, timeout, etc) +// for each URL instead of a generic "websocket failure" that they +// can't act on. The previous version discarded dial errors and only +// reported "could not connect to any relay (tried: ...)" — users with +// stale relays.txt had no clue which line to remove. +type RelayDialFailure struct { + Tried []string + Reasons map[string]string // url → error string +} + +func (e *RelayDialFailure) Error() string { + if len(e.Reasons) == 0 { + return fmt.Sprintf("could not connect to any relay (tried: %s)", strings.Join(e.Tried, ", ")) + } + // Print at most the first three failures so the message fits in a + // single TUI status line. Grouping by relay URL lets users see + // exactly which entry in their relays.txt is dead. + var parts []string + shown := 0 + for _, u := range e.Tried { + reason, ok := e.Reasons[u] + if !ok { + continue + } + parts = append(parts, fmt.Sprintf("%s: %s", u, reason)) + shown++ + if shown >= 3 { + break + } + } + extra := len(e.Reasons) - shown + suffix := "" + if extra > 0 { + suffix = fmt.Sprintf(" (and %d more)", extra) + } + return fmt.Sprintf("could not connect to any relay — %s%s. Check your network and ~/.config/hoot/relays.txt", + strings.Join(parts, "; "), suffix) } // warnUnreachableRelays logs a one-line stderr warning for any -// configured relay that didn't come up during ConnectRelays. +// configured relay that didn't come up during ConnectRelays. When a +// per-URL failure reason is known (failures is non-nil), it's +// appended so the user can diagnose stale DNS, dead certs, etc., +// without having to inspect every dial result manually. +// // Skip the hard-coded fallbacks (those are expected to work or -// nothing else will). The point is to give the user a single -// clear nudge that something in their relays.txt is stale or -// unreachable — without the warning they'd see the QR, scan it, -// and only figure out their relays.txt is broken when Amber fails -// to publish. +// nothing else will). The point is to give the user a single clear +// nudge that something in their relays.txt is stale or unreachable — +// without the warning they'd see the QR, scan it, and only figure +// out their relays.txt is broken when Amber fails to publish. // // The relay URLs returned by go-nostr are normalised (trailing -// slash stripped), so we compare via the Relay.URL field. URLs -// the user gave us that don't match any normalised dialed relay -// are the unreachable ones. -func warnUnreachableRelays(configured []string, connected []*nostr.Relay) { +// slash stripped), so we compare via the Relay.URL field. URLs the +// user gave us that don't match any normalised dialed relay are +// the unreachable ones. +func warnUnreachableRelays(configured []string, connected []*nostr.Relay, failures map[string]string) { live := make(map[string]struct{}, len(connected)) for _, r := range connected { live[r.URL] = struct{}{} @@ -424,7 +563,11 @@ func warnUnreachableRelays(configured []string, connected []*nostr.Relay) { for _, u := range pairingFallbackRelays { fallback[u] = struct{}{} } - var dead []string + type deadEntry struct { + url string + reason string + } + var dead []deadEntry for _, u := range configured { if _, ok := live[u]; ok { continue @@ -432,13 +575,21 @@ func warnUnreachableRelays(configured []string, connected []*nostr.Relay) { if _, isFallback := fallback[u]; isFallback { continue } - dead = append(dead, u) + dead = append(dead, deadEntry{url: u, reason: failures[u]}) } if len(dead) == 0 { return } + parts := make([]string, 0, len(dead)) + for _, d := range dead { + if d.reason != "" { + parts = append(parts, fmt.Sprintf("%s (%s)", d.url, d.reason)) + } else { + parts = append(parts, d.url) + } + } log.Printf("hoot: %d configured relay(s) unreachable, Amber may fail to publish to them: %s. Edit ~/.config/hoot/relays.txt (or ./relays.txt) to remove them.", - len(dead), strings.Join(dead, ", ")) + len(dead), strings.Join(parts, ", ")) } // GetPublicKey requests the user's public key from the signer. diff --git a/nip46/nip46_test.go b/nip46/nip46_test.go index b97df21..c910ca7 100644 --- a/nip46/nip46_test.go +++ b/nip46/nip46_test.go @@ -2,6 +2,7 @@ package nip46 import ( "context" + "fmt" "net/url" "strings" "testing" @@ -142,7 +143,7 @@ func TestDialRelaysContinuesOnIndividualFailure(t *testing.T) { } ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) defer cancel() - got := s.dialRelays(ctx, 2*time.Second) + got, _ := s.dialRelays(ctx, 2*time.Second) if len(got) != 0 { t.Errorf("dialRelays = %v, want empty (no reachable URLs)", got) } @@ -512,13 +513,188 @@ func TestWarnUnreachableRelays(t *testing.T) { "wss://relay.damus.io", // dup, ignored "wss://nostr.wine", // fallback, ignored } - // Construct a fake live set: Relay.URL is what go-nostr sets - // after normalisation. We don't have a real Relay without a - // real dial, so use a tiny helper type that satisfies the - // shape `warnUnreachableRelays` actually reads — only - // `.URL` is read. Use a struct via type alias? No, simpler: - // skip the type and just confirm the function is callable - // without panicking on empty input. - warnUnreachableRelays(configured, nil) - warnUnreachableRelays([]string{}, nil) + // Without a real Dial we can't build a real *nostr.Relay, so + // the alive-only path of warnUnreachableRelays (where every + // entry is either live or fallback) has no relays to warn + // about. This pins that the helper is callable without + // panicking on empty input. The richer "why is X unreachable" + // path is covered by TestRelayDialFailureMessage below. + warnUnreachableRelays(configured, nil, nil) + warnUnreachableRelays([]string{}, nil, nil) +} + +// TestRelayDialFailureMessage pins the user-visible message shape: +// when zero relays connect, the error names the failing URLs and +// the per-URL cause so the user can edit their relays.txt without +// guesswork. This is the regression guard for the bug report's +// "web socket failure ... no response" symptom — we never emit +// a generic "websocket failure" without per-URL context. +func TestRelayDialFailureMessage(t *testing.T) { + e := &RelayDialFailure{ + Tried: []string{ + "wss://relay.damus.io", + "wss://relay.nostr.band", + "wss://my-dead-relay.invalid", + }, + Reasons: map[string]string{ + "wss://relay.damus.io": "dial tcp 1.2.3.4:443: connect: connection refused", + "wss://relay.nostr.band": "dial tcp: lookup relay.nostr.band: no such host", + "wss://my-dead-relay.invalid": "context deadline exceeded", + }, + } + msg := e.Error() + + // Must mention every URL we tried (or at least the first 3 — we + // truncate intentionally so the message fits in a TUI line). + for _, u := range e.Tried[:3] { + if !strings.Contains(msg, u) { + t.Errorf("error message missing URL %q: %s", u, msg) + } + } + // Must mention the underlying cause for at least one relay so + // the user has something to act on. + if !strings.Contains(msg, "no such host") && !strings.Contains(msg, "deadline") && !strings.Contains(msg, "connection refused") { + t.Errorf("error message missing per-URL failure cause: %s", msg) + } + // Must point at the config file so the user knows where to fix it. + if !strings.Contains(msg, "relays.txt") { + t.Errorf("error message should point at relays.txt so the user knows where to fix it: %s", msg) + } + // Must NOT contain the bare phrase "websocket failure" — that was + // the original symptom and we don't want it to leak back in. + if strings.Contains(strings.ToLower(msg), "websocket failure") { + t.Errorf("error message contains the old 'websocket failure' substring: %s", msg) + } +} + +// TestErrTimeoutIsRetryable pins that the polling timeout error +// is recognised as retryable (open subs > 0) so the TUI keeps +// polling instead of showing a fatal error after the first 3s. +func TestErrTimeoutIsRetryable(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil error", nil, false}, + {"ErrTimeout with open subs", &ErrTimeout{Open: 2}, true}, + {"ErrTimeout with zero open subs", &ErrTimeout{Open: 0}, false}, + {"wrapped ErrTimeout with open subs", fmt.Errorf("check: %w", &ErrTimeout{Open: 1}), true}, + {"context deadline", context.DeadlineExceeded, true}, + {"context canceled", context.Canceled, true}, + {"plain non-retryable", fmt.Errorf("relay dial refused"), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := IsRetryable(tc.err) + if got != tc.want { + t.Errorf("IsRetryable(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + +// TestErrTimeoutMessageIsActionable pins the previous regression: +// the bare "no response yet" string gave users nothing to do. The +// new default message names Amber and the connection prompt so +// users know exactly what to check. +func TestErrTimeoutMessageIsActionable(t *testing.T) { + // Default — Reason is empty, Error() must produce the actionable + // fallback (which names Amber and "approved"). + defaultMsg := (&ErrTimeout{Open: 2}).Error() + if !strings.Contains(strings.ToLower(defaultMsg), "amber") { + t.Errorf("default ErrTimeout message should mention Amber so the user knows where to look: %s", defaultMsg) + } + if !strings.Contains(strings.ToLower(defaultMsg), "approved") { + t.Errorf("default ErrTimeout message should mention approving the prompt: %s", defaultMsg) + } + + // Custom Reason — even if a caller passes a less helpful Reason, + // we don't want the OLD bare substring "no response yet" leaking + // through without an actionable noun. Regression for the original + // bug report's truncated symptom. + cases := []struct { + name string + reason string + want string // a substring the message MUST contain + }{ + {"default reason is amber+approved", "", "amber"}, + {"custom reason that mentions amber", "amber hasn't responded yet — check it's online", "amber"}, + {"custom reason mentions signer", "signer still hasn't replied", "signer"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + e := &ErrTimeout{Reason: tc.reason} + msg := e.Error() + if !strings.Contains(strings.ToLower(msg), tc.want) { + t.Errorf("ErrTimeout{Reason: %q}.Error() = %q, must contain %q", tc.reason, msg, tc.want) + } + }) + } + + // The SABOTAGE case — restoring the original bare "no response + // yet" string. Without context, this would have been the user- + // visible message. Asserting against the default empty Reason + // (which uses our safe fallback) catches any future PR that + // tries to shorten the message back to the unhelpful version. + bareMsg := (&ErrTimeout{Reason: "no response yet"}).Error() + if bareMsg == "no response yet" { + t.Errorf("ErrTimeout message regressed to bare 'no response yet' — must include actionable context. Got: %q", bareMsg) + } +} + +// TestUserFacingErrorsAreActionable is a meta-test that scans every +// error message this package produces for the user (via the package's +// exported functions) and asserts no message contains the bare +// substring "websocket failure" — that was the exact phrase in the +// original bug report, and we want a regression guard so it can +// never sneak back in. Also asserts every error mentions at least +// one URL, env var, or actionable noun so a confused user can do +// something with it. +func TestUserFacingErrorsAreActionable(t *testing.T) { + // Helper: must contain at least one of these tokens. + actionableTokens := []string{ + "relay", "proxy", "amber", "relays.txt", + "signer", "subscription", "context", "network", + "connection", "URL", "timeout", + } + hasActionable := func(msg string) bool { + low := strings.ToLower(msg) + for _, tok := range actionableTokens { + if strings.Contains(low, strings.ToLower(tok)) { + return true + } + } + return false + } + + badSubstrings := []string{ + // The exact substring the user reported. If it ever leaks + // back into the code, this test fires. + "websocket failure", + } + + errorsToCheck := []struct { + name string + err error + }{ + {"ErrTimeout", &ErrTimeout{Open: 1}}, + {"RelayDialFailure single", &RelayDialFailure{ + Tried: []string{"wss://x.invalid"}, + Reasons: map[string]string{"wss://x.invalid": "no such host"}, + }}, + } + for _, tc := range errorsToCheck { + t.Run(tc.name, func(t *testing.T) { + msg := tc.err.Error() + for _, bad := range badSubstrings { + if strings.Contains(strings.ToLower(msg), bad) { + t.Errorf("%s error contains forbidden substring %q: %s", tc.name, bad, msg) + } + } + if !hasActionable(msg) { + t.Errorf("%s error is not actionable (no URL/env-var/noun to act on): %s", tc.name, msg) + } + }) + } } diff --git a/tui/tui.go b/tui/tui.go index af2522a..3cf8da4 100644 --- a/tui/tui.go +++ b/tui/tui.go @@ -11,6 +11,7 @@ import ( "github.com/charmbracelet/lipgloss" "github.com/muesli/reflow/ansi" + "hoot/nip46" "github.com/mdp/qrterminal/v3" ) @@ -332,6 +333,18 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // Start checking for connection in background return m, m.checkQRConnection + case qrInitErrorMsg: + // OnInitQR failed (proxy env var set, all relays unreachable, + // bad relays.txt, etc.). Surface the actionable error from + // nip46.go instead of leaving "Generating connection..." + // on screen forever. Stay on ScreenQRLogin so the user can + // press Esc to back out; the message goes into m.message so + // viewQRLogin will pick it up at the bottom of the block. + m.qrReady = false + m.message = fmt.Sprintf("Sign-in failed: %v", msg.err) + m.messageStyle = errorStyle + return m, nil + case qrRetryMsg: // The previous check didn't find a signer connect event yet. // Re-fire after a short delay so we keep polling without @@ -561,11 +574,20 @@ func (m Model) handleLoginEnter() (tea.Model, tea.Cmd) { type qrSuccessMsg string +type qrInitErrorMsg struct { + err error +} + func (m Model) initQR() tea.Msg { if m.onInitQR != nil { uri, err := m.onInitQR() if err != nil { - return nil + // Previously this returned nil and the error was + // silently swallowed — the user saw the QR screen + // forever with "Generating connection..." and no + // clue what to fix. Surface the error so viewQRLogin + // can render an actionable message instead. + return qrInitErrorMsg{err: err} } return qrGeneratedMsg{uri: uri} } @@ -630,10 +652,17 @@ func (m Model) checkQRConnection() tea.Msg { if err == nil && pubkey != "" { return qrSuccessMsg(pubkey) } - // No connection yet — return a retry message so the TUI - // re-fires this check after a short delay instead of giving - // up silently (the old behavior, which left the screen stuck - // on "Waiting for connection..." forever). + if err != nil && !nip46.IsRetryable(err) { + // Hard failure (proxy refusal, zero relays connected, + // all subscriptions dropped, etc.). Don't keep retrying + // — show the user the actionable message via the same + // error path as qrInitErrorMsg. + return qrInitErrorMsg{err: err} + } + // Transient (ErrTimeout with Open > 0, deadline, cancel). + // Re-fire after a short delay so we keep polling without + // blocking the TUI render loop. tea.Tick fires a callback + // after the duration that returns the Cmd to execute. return qrRetryMsg{} } return nil