From 98b4106c3f1d9199ed56354491f55d9991367822 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 8 May 2026 17:35:12 -0400 Subject: [PATCH 1/2] fix(recap): surface real auth/network errors instead of "sign in" hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The auth pre-check in runRecap collapsed three distinct failure modes (missing token, keyring read error, insecure base URL) into a single "Sign in with `entire login`" message, with the real error swallowed by NewSilentError. This made flags appear broken on first run because the function exited before any line that consumed --week, --agent, etc. Replace the gating call with a newRecapClient helper that builds the client even with an empty token, so FetchMeRecap always runs and surfaces the actual cause via the existing recapLoadErrorMessage mapping (401 → "Run `entire login` to re-authenticate.", 4xx/5xx → specific guidance). The insecure-URL check stays but only when a token exists, with a friendlier message that names the offending env var and the fix. Also split DNS NXDOMAIN out of the generic network-error path so a misconfigured ENTIRE_API_BASE_URL says "Could not resolve API host..." instead of blaming the user's internet connection. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: d534f23017c5 --- cmd/entire/cli/recap.go | 23 ++++++++++++++++++++--- cmd/entire/cli/recap_errors.go | 13 +++++++++++++ cmd/entire/cli/recap_test.go | 19 +++++++++++++++++++ 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/cmd/entire/cli/recap.go b/cmd/entire/cli/recap.go index f7a6b51f1e..ed03ef6ee5 100644 --- a/cmd/entire/cli/recap.go +++ b/cmd/entire/cli/recap.go @@ -15,6 +15,7 @@ import ( "golang.org/x/term" "github.com/entireio/cli/cmd/entire/cli/api" + "github.com/entireio/cli/cmd/entire/cli/auth" "github.com/entireio/cli/cmd/entire/cli/gitremote" "github.com/entireio/cli/cmd/entire/cli/interactive" "github.com/entireio/cli/cmd/entire/cli/paths" @@ -122,10 +123,13 @@ func runRecap(ctx context.Context, w, errW io.Writer, f *recapFlags) error { if err != nil { return err } - client, err := NewAuthenticatedAPIClient(f.insecureHTTP) + client, err := newRecapClient(f.insecureHTTP) if err != nil { - fmt.Fprintln(errW, "Sign in with `entire login` to use `entire recap`.") - return NewSilentError(err) + if errors.Is(err, api.ErrInsecureHTTP) { + fmt.Fprintf(errW, "ENTIRE_API_BASE_URL is set to an insecure http:// URL (%s). Use https:// for production, or pass --insecure-http-auth for local dev.\n", api.BaseURL()) + return NewSilentError(err) + } + return err } rangeKey := f.rangeKey() repoSlug := currentRepoSlug(ctx) @@ -154,6 +158,19 @@ func runRecap(ctx context.Context, w, errW io.Writer, f *recapFlags) error { return nil } +// newRecapClient does not gate on a missing token; FetchMeRecap surfaces 401s +// via recapLoadErrorMessage so flag effects (--week, --agent, ...) and the +// real auth error are not collapsed into one "sign in" hint. +func newRecapClient(insecureHTTP bool) (*api.Client, error) { + token, _ := auth.LookupCurrentToken() //nolint:errcheck // keyring errors are non-fatal here; the API call surfaces auth failure with the right hint + if token != "" && !insecureHTTP { + if err := api.RequireSecureURL(api.BaseURL()); err != nil { + return nil, fmt.Errorf("base URL check: %w", err) + } + } + return api.NewClient(token), nil +} + func handleRecapFetchError(w io.Writer, err error) error { if shouldShowRecapLoadErrorMessage(err) { fmt.Fprintln(w, recapLoadErrorMessage(err)) diff --git a/cmd/entire/cli/recap_errors.go b/cmd/entire/cli/recap_errors.go index 9d650a5c22..7f13853502 100644 --- a/cmd/entire/cli/recap_errors.go +++ b/cmd/entire/cli/recap_errors.go @@ -37,12 +37,25 @@ func recapLoadErrorMessage(err error) string { return err.Error() } } + if host, ok := dnsNotFoundHost(err); ok { + return fmt.Sprintf("Could not resolve API host %q (DNS lookup failed). Check ENTIRE_API_BASE_URL — the host may be misspelled or the env var may be pointing at a non-existent server. Details: %v", host, err) + } if isRecapNetworkError(err) { return fmt.Sprintf("Could not reach entire.io. Check your internet connection and ENTIRE_API_BASE_URL if you use a custom API host. Details: %v", err) } return err.Error() } +// dnsNotFoundHost reports an NXDOMAIN-style failure, distinguishing "host +// doesn't exist" from generic connectivity loss. +func dnsNotFoundHost(err error) (string, bool) { + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) && dnsErr.IsNotFound { + return dnsErr.Name, true + } + return "", false +} + func recapErrorDetail(err *api.HTTPError) string { if strings.TrimSpace(err.Message) != "" { return fmt.Sprintf("HTTP %d: %s", err.StatusCode, err.Message) diff --git a/cmd/entire/cli/recap_test.go b/cmd/entire/cli/recap_test.go index 60b63ea7b9..7e27adca3c 100644 --- a/cmd/entire/cli/recap_test.go +++ b/cmd/entire/cli/recap_test.go @@ -314,6 +314,25 @@ func TestRecapLoadErrorMessage_NetworkError(t *testing.T) { } } +func TestRecapLoadErrorMessage_DNSNotFound(t *testing.T) { + t.Parallel() + + nxdomain := &net.DNSError{Name: "no-token-here.example.com", Err: "no such host", IsNotFound: true} + got := recapLoadErrorMessage(fmt.Errorf("me/recap get: %w", nxdomain)) + if strings.Contains(got, "Check your internet connection") { + t.Fatalf("NXDOMAIN should not blame internet connection:\n%s", got) + } + for _, want := range []string{ + "Could not resolve API host", + "no-token-here.example.com", + "ENTIRE_API_BASE_URL", + } { + if !strings.Contains(got, want) { + t.Fatalf("message missing %q:\n%s", want, got) + } + } +} + func TestRecapLoadErrorMessage_ContextCancellation(t *testing.T) { t.Parallel() From da6a6d4d2a47de81d57538277ee7121094f155c0 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 11 May 2026 13:11:21 -0400 Subject: [PATCH 2/2] fix(recap): surface keyring read errors with targeted message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review on #1168: newRecapClient was discarding the error from auth.LookupCurrentToken, which silently turned a keyring read failure (locked, permission denied, etc.) into a "no token" state. That re-introduced the same collapsed-error UX the PR set out to fix — the user would be told to run `entire login`, which can't help when the keyring itself is the problem. Wrap the underlying error in a *keyringReadError (preserving the cause via errors.As) and route it in runRecap to a targeted message that states the keyring is the problem and that re-login is unlikely to help. Truly-missing tokens (keyring.ErrNotFound resolves to "", nil upstream) still flow through to FetchMeRecap and the existing 401 → "Run `entire login` to re-authenticate." mapping. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 60d4f6868ab0 --- cmd/entire/cli/recap.go | 30 +++++++++++++++++++++++++----- cmd/entire/cli/recap_test.go | 21 +++++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/cmd/entire/cli/recap.go b/cmd/entire/cli/recap.go index ed03ef6ee5..5271ef543d 100644 --- a/cmd/entire/cli/recap.go +++ b/cmd/entire/cli/recap.go @@ -125,11 +125,16 @@ func runRecap(ctx context.Context, w, errW io.Writer, f *recapFlags) error { } client, err := newRecapClient(f.insecureHTTP) if err != nil { - if errors.Is(err, api.ErrInsecureHTTP) { + var keyringErr *keyringReadError + switch { + case errors.Is(err, api.ErrInsecureHTTP): fmt.Fprintf(errW, "ENTIRE_API_BASE_URL is set to an insecure http:// URL (%s). Use https:// for production, or pass --insecure-http-auth for local dev.\n", api.BaseURL()) - return NewSilentError(err) + case errors.As(err, &keyringErr): + fmt.Fprintf(errW, "Could not read your auth token from the system keyring: %v. Running `entire login` may not help — the keyring may be locked or inaccessible. Check your OS keychain settings.\n", keyringErr.Cause) + default: + return err } - return err + return NewSilentError(err) } rangeKey := f.rangeKey() repoSlug := currentRepoSlug(ctx) @@ -158,11 +163,26 @@ func runRecap(ctx context.Context, w, errW io.Writer, f *recapFlags) error { return nil } +// keyringReadError marks a failure to read the auth token from the system +// keyring (locked, permission denied, etc.) — distinct from "no token saved", +// which keyring.ErrNotFound resolves to (token=="", err==nil) upstream. +type keyringReadError struct{ Cause error } + +func (e *keyringReadError) Error() string { + return "read auth token from keyring: " + e.Cause.Error() +} +func (e *keyringReadError) Unwrap() error { return e.Cause } + // newRecapClient does not gate on a missing token; FetchMeRecap surfaces 401s // via recapLoadErrorMessage so flag effects (--week, --agent, ...) and the -// real auth error are not collapsed into one "sign in" hint. +// real auth error are not collapsed into one "sign in" hint. A keyring read +// failure is surfaced as *keyringReadError so the caller can show a targeted +// message instead of misattributing it to a missing login. func newRecapClient(insecureHTTP bool) (*api.Client, error) { - token, _ := auth.LookupCurrentToken() //nolint:errcheck // keyring errors are non-fatal here; the API call surfaces auth failure with the right hint + token, err := auth.LookupCurrentToken() + if err != nil { + return nil, &keyringReadError{Cause: err} + } if token != "" && !insecureHTTP { if err := api.RequireSecureURL(api.BaseURL()); err != nil { return nil, fmt.Errorf("base URL check: %w", err) diff --git a/cmd/entire/cli/recap_test.go b/cmd/entire/cli/recap_test.go index 7e27adca3c..69cfff5a31 100644 --- a/cmd/entire/cli/recap_test.go +++ b/cmd/entire/cli/recap_test.go @@ -143,6 +143,27 @@ func TestRecapFlags_ColorEnabled(t *testing.T) { } } +func TestKeyringReadError_PreservesCauseAndMatchesAs(t *testing.T) { + t.Parallel() + + cause := errors.New("keychain locked") + err := error(&keyringReadError{Cause: cause}) + + if !errors.Is(err, cause) { + t.Fatalf("errors.Is should match wrapped cause; got false for %v", err) + } + var keyringErr *keyringReadError + if !errors.As(err, &keyringErr) { + t.Fatalf("errors.As should extract *keyringReadError; got false for %v", err) + } + if !errors.Is(keyringErr.Cause, cause) { + t.Fatalf("Cause = %v, want %v", keyringErr.Cause, cause) + } + if !strings.Contains(err.Error(), "keychain locked") { + t.Fatalf("Error() should include cause text; got %q", err.Error()) + } +} + func TestRunRecap_PrerequisiteErrorsUseErrorWriter(t *testing.T) { tmpDir := t.TempDir() t.Chdir(tmpDir)