From c557cbceecb455c56725372b53c01eb7e00277fc Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 9 Oct 2026 17:52:33 +0200 Subject: [PATCH] Drop the BFF fallback for activity and recap entire activity and entire recap tried the caller's home entire-api cell first and fell back to the data API (the BFF) when the cell client could not be built. The BFF serves /me/activity, /me/commits and /me/recap by proxying to that same home cell (proxyHomeMe), so the fallback could never succeed where the cell path failed; it only added a hop and hid the real error. Both commands now use the home cell only. Client errors go through renderDataAPIAuthError: no login prints the login hint (recap used to render the BFF's 401 instead), cancellation stays silent, and anything else, including a home jurisdiction without a cell, surfaces verbatim. runAuthenticatedDataAPI, logCellClientFallback and currentRepoSlug lose their last callers and are removed. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4GNVFC303Y9TKGWSAWCAKBN --- cmd/entire/cli/activity_cmd_test.go | 15 +++-- cmd/entire/cli/auth/cell_data_api.go | 9 ++- cmd/entire/cli/authcmd.go | 14 ----- cmd/entire/cli/entireapi_client.go | 38 +++--------- cmd/entire/cli/recap.go | 89 ++++++---------------------- cmd/entire/cli/recap_test.go | 34 ++++++++++- docs/development/api-routing.md | 6 +- 7 files changed, 76 insertions(+), 129 deletions(-) diff --git a/cmd/entire/cli/activity_cmd_test.go b/cmd/entire/cli/activity_cmd_test.go index 62d23cf7d5..7f13f75e9b 100644 --- a/cmd/entire/cli/activity_cmd_test.go +++ b/cmd/entire/cli/activity_cmd_test.go @@ -15,6 +15,7 @@ import ( "github.com/entireio/cli/internal/entireclient/clusterdiscovery" "github.com/entireio/cli/internal/entireclient/contexts" "github.com/entireio/cli/internal/entireclient/tokenstore" + "github.com/entireio/cli/internal/entireclient/userdirs" ) func strPtr(v string) *string { return &v } @@ -37,14 +38,17 @@ func strPtr(v string) *string { return &v } // from an internal resolver timeout that fires on its own derived context // while the caller's ctx is still live. func TestRunActivity_SilencesContextCanceled(t *testing.T) { - // No t.Parallel: SetResolveContextForAPIForTest mutates package-level + // No t.Parallel: SetResolveContextForCellAPIForTest mutates package-level // auth state. // // Simulate the user hitting Ctrl+C during auth resolution: the // cancellation surfaces from the discovery fetch, and runActivity must // silence it rather than mislabel it "Not logged in". + unsetEnv(t, auth.EnvTokenVar) + t.Setenv(userdirs.EnvConfigDir, t.TempDir()) + t.Setenv(userdirs.EnvCacheHome, t.TempDir()) t.Setenv(api.BaseURLEnvVar, "https://entire.io") - t.Cleanup(auth.SetResolveContextForAPIForTest(t, + t.Cleanup(auth.SetResolveContextForCellAPIForTest(t, func(context.Context, string, string, string, *http.Client, clusterdiscovery.DebugFunc) (*contexts.Context, error) { return nil, context.Canceled })) @@ -74,15 +78,18 @@ func TestRunActivity_SilencesContextCanceled(t *testing.T) { // hint and a SilentError so the raw "not logged in" string doesn't // also print via cobra. func TestRunActivity_PrintsLoginHintOnNotLoggedIn(t *testing.T) { - // No t.Parallel: SetResolveContextForAPIForTest mutates package-level + // No t.Parallel: SetResolveContextForCellAPIForTest mutates package-level // auth state. // // Discovery selects a context whose keyring slot holds nothing, so the // per-context provider reports ErrNotLoggedIn. + unsetEnv(t, auth.EnvTokenVar) + t.Setenv(userdirs.EnvConfigDir, t.TempDir()) + t.Setenv(userdirs.EnvCacheHome, t.TempDir()) t.Setenv(api.BaseURLEnvVar, "https://entire.io") t.Cleanup(tokenstore.UseFileBackendForTesting(filepath.Join(t.TempDir(), "tokens.json"))) c := &contexts.Context{Name: "me@core", CoreURL: "https://core.example", Handle: "me", KeychainService: "kc:me"} - t.Cleanup(auth.SetResolveContextForAPIForTest(t, + t.Cleanup(auth.SetResolveContextForCellAPIForTest(t, func(context.Context, string, string, string, *http.Client, clusterdiscovery.DebugFunc) (*contexts.Context, error) { return c, nil })) diff --git a/cmd/entire/cli/auth/cell_data_api.go b/cmd/entire/cli/auth/cell_data_api.go index 15df5f076d..c46f01480f 100644 --- a/cmd/entire/cli/auth/cell_data_api.go +++ b/cmd/entire/cli/auth/cell_data_api.go @@ -531,11 +531,10 @@ type clusterListingRow struct { // ErrNoCellForJurisdiction signals that the requested jurisdiction — the // caller's home, or an explicit --jurisdiction — has no entire-api cell in the -// login core's cluster catalog (or its row carries no apiUrl). It is not fatal: -// callers that also have a data-API path (e.g. activity/recap) treat it as -// "entire-api isn't serving this region yet" and fall back rather than failing -// the command. errors.Is unwraps it from the contextual message, which names -// the core consulted and the jurisdictions it does serve. +// login core's cluster catalog (or its row carries no apiUrl). Search treats it +// as a skipped cell rather than a failed query; single-cell callers such as +// activity and recap surface it. errors.Is unwraps it from the contextual +// message, which names the core consulted and the jurisdictions it does serve. var ErrNoCellForJurisdiction = errors.New("no entire-api cell configured for jurisdiction") // resolveCellAPIBaseURL is the catalog cell resolver: it lists the clusters of diff --git a/cmd/entire/cli/authcmd.go b/cmd/entire/cli/authcmd.go index 43d1af6983..4339721563 100644 --- a/cmd/entire/cli/authcmd.go +++ b/cmd/entire/cli/authcmd.go @@ -7,23 +7,9 @@ import ( "io" "strings" - "github.com/entireio/cli/cmd/entire/cli/api" "github.com/entireio/cli/cmd/entire/cli/auth" ) -// runAuthenticatedDataAPI centralizes the auth gate for commands that must -// call the Entire data API as the current user. Keep intentionally anonymous -// flows (for example recap's server-rendered 401 path) out of this helper. -func runAuthenticatedDataAPI(ctx context.Context, errW io.Writer, insecureHTTP bool, fn func(context.Context, *api.Client) error) error { - client, err := NewAuthenticatedAPIClient(ctx, insecureHTTP) - if err != nil { - // No repo to name: this gate builds the generic data-API client, which - // resolves no repo and so can never produce errRepoNotOnboarded. - return renderDataAPIAuthError(ctx, errW, "", err) - } - return fn(ctx, client) -} - // renderDataAPIAuthError decides whether err should print or stay silent. // Silence is reserved for the caller's own context actually firing (checked // via ctx.Err(), not errors.Is(err, ...)): a resolver called from inside err's diff --git a/cmd/entire/cli/entireapi_client.go b/cmd/entire/cli/entireapi_client.go index f73bb28b99..5fa97d9485 100644 --- a/cmd/entire/cli/entireapi_client.go +++ b/cmd/entire/cli/entireapi_client.go @@ -2,7 +2,6 @@ package cli import ( "context" - "errors" "io" "strings" "time" @@ -10,7 +9,6 @@ import ( "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/logging" "github.com/entireio/cli/internal/coreapi" ) @@ -19,42 +17,20 @@ import ( // it), so a stalled core must not hang the command — mirror cellResolveTimeout. const currentRepoRefTimeout = 5 * time.Second -// runAuthenticatedActivityAPI runs fn with an authenticated client for the -// activity/recap surface. It prefers the caller's home entire-api cell (the same -// shared client the experts commands use), which serves the /me/* endpoints -// these commands call. -// -// Cell routing is a best-effort upgrade: any failure building the cell client — -// the region has no cell yet (ErrNoCellForJurisdiction), not logged in, or a -// discovery/exchange error — falls back to the data API, which also serves -// /me/* and yields the canonical auth errors (e.g. the "not logged in" hint). -// This keeps the migration transparent and non-regressive; non-obvious -// fallbacks are logged for diagnosis. Both backends expose the same /me/* paths, -// so fn is agnostic to which client it receives. -// -// Both paths act as the same login — auth.ResolveDataAPI applies the cell -// path's precedence (ENTIRE_TOKEN, else the selected context) — so the -// fallback never changes identity or environment. +// runAuthenticatedActivityAPI runs fn with a client for the caller's home +// entire-api cell, which serves the /me/* endpoints the activity command calls. +// There is no data-API fallback: the BFF answers /me/* by proxying to that same +// home cell, so a caller the cell path cannot serve gets nothing better there. +// Client-construction failures go through renderDataAPIAuthError, which turns +// ErrNotLoggedIn into the login hint and silences the caller's own cancellation. func runAuthenticatedActivityAPI(ctx context.Context, errW io.Writer, insecureHTTP bool, fn func(context.Context, *api.Client) error) error { client, err := auth.NewEntireAPICellClient(ctx, insecureHTTP, nil) if err != nil { - logCellClientFallback(ctx, err) - return runAuthenticatedDataAPI(ctx, errW, insecureHTTP, fn) + return renderDataAPIAuthError(ctx, errW, "", err) } return fn(ctx, client) } -// logCellClientFallback records, at debug, that an activity/recap command fell -// back from the entire-api cell to the data API. The expected cases — the -// region has no cell yet, or the caller isn't logged in — aren't logged: they -// are normal during rollout and on first use, not diagnosable failures. -func logCellClientFallback(ctx context.Context, err error) { - if errors.Is(err, auth.ErrNoCellForJurisdiction) || errors.Is(err, auth.ErrNotLoggedIn) { - return - } - logging.Debug(ctx, "activity/recap: entire-api cell client unavailable, using data API", "error", err.Error()) -} - // forgeToMirrorProvider maps a gitremote forge identifier (e.g. "gh") to the // upstream provider the control plane records mirrors under (e.g. "github"). // entire-api routing only supports GitHub mirrors today. diff --git a/cmd/entire/cli/recap.go b/cmd/entire/cli/recap.go index 0828a3e5d5..7c0580c5ae 100644 --- a/cmd/entire/cli/recap.go +++ b/cmd/entire/cli/recap.go @@ -138,13 +138,10 @@ func runRecap(ctx context.Context, w, errW io.Writer, f *recapFlags) error { fmt.Fprintf(errW, "%s. Use https:// for production, or pass --insecure-http-auth for local dev.\n", insecureDataOverrideNote()) return NewSilentError(err) } - // Token resolution can fail for many reasons unrelated to the - // keyring — STS exchange rejected, network error, audience - // misconfiguration. Surface the underlying error verbatim - // rather than misattributing it to a missing or locked - // keyring entry; main.go's default printer is honest about - // what went wrong. - return err + // Not logged in gets the login hint; every other failure (refresh + // rejected, network error, no cell for the home jurisdiction) surfaces + // verbatim rather than being misattributed to a missing login. + return renderDataAPIAuthError(ctx, errW, "", err) } rangeKey := f.rangeKey() if f.useTUI(interactive.IsTerminalWriter(w), interactive.CanPromptInteractively(), IsAccessibleMode()) { @@ -174,65 +171,23 @@ 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. -// -// Goes through auth.ResolveDataAPI (the same login-following path as -// activity/search/dispatch). ErrNotLoggedIn is collapsed back into an empty -// token so the caller's "render with no bearer, let the server respond 401" -// path still fires. Every other resolution failure (refresh rejected, network -// error, keyring locked) surfaces verbatim to the caller. -// newRecapClient returns the recap client, the value to pass as /me/recap's -// ?repo= (its team/contributors scope), and the repo's owner/repo display name. -// The scope is the current repo's ULID when routed to an entire-api cell (which -// addresses repos by id), or its owner/repo slug on the data API (which -// addresses them by name); both come from one remote/mirror resolution, so the -// caller never re-resolves for display. Empty when the current repo can't be -// resolved — recap then shows the personal side only. +// newRecapClient returns a client for the caller's home entire-api cell, the +// value to pass as /me/recap's ?repo= (the current repo's ULID, its +// team/contributors scope), and the repo's owner/repo display name. Both are +// empty when the current repo can't be resolved; recap then shows the personal +// side only. // -// It prefers the caller's home entire-api cell (the shared client) and falls -// back to the data API on a cell-client failure — the cell path is a -// best-effort upgrade, so a cell problem must never break a command that worked -// before it existed. Expected fallbacks (region has no cell yet, not logged in) -// are silent; unexpected ones are debug-logged (logCellClientFallback). Only -// failures of the data-API path itself surface — except ErrNotLoggedIn, which -// recap tolerates, rendering and letting the server answer 401. Both paths -// act as the same login (auth.ResolveDataAPI mirrors the cell path's -// precedence), so a selected staging login whose refresh failed is reported, -// never answered by a production login. +// There is no data-API fallback: the BFF answers /me/recap by proxying to that +// same home cell, so a caller the cell path cannot serve gets nothing better +// there. Client-construction errors (not logged in, no cell for the home +// jurisdiction, refresh rejected) are returned for runRecap to render. func newRecapClient(ctx context.Context, insecureHTTP bool) (client *api.Client, repoScope, repoName string, err error) { - cellClient, cellErr := auth.NewEntireAPICellClient(ctx, insecureHTTP, nil) - if cellErr == nil { - repoID, repoSlug := currentRepoRef(ctx) - return cellClient, repoID, repoSlug, nil - } - logCellClientFallback(ctx, cellErr) - - if insecureHTTP { - auth.EnableInsecureHTTP() - } else if err := requireSecureDataOverride(); err != nil { - return nil, "", "", err - } - target, err := auth.ResolveDataAPI(ctx) - if errors.Is(err, auth.ErrNotLoggedIn) { - target, err = auth.DataAPI{BaseURL: api.BaseURL()}, nil - if base, berr := auth.DataBaseURL(); berr == nil { - target.BaseURL = base - } - } + client, err = auth.NewEntireAPICellClient(ctx, insecureHTTP, nil) if err != nil { - return nil, "", "", err + return nil, "", "", err //nolint:wrapcheck // auth returns contextual, user-facing errors } - // No second scheme check: requireSecureDataOverride above already rejected an - // http ENTIRE_API_BASE_URL, and every other value target.BaseURL can hold is - // built by auth.dataBaseURLForCore as "https://" + site. The one that mattered - // is the one that runs before credentials are resolved — - // TestNewRecapClient_RejectsInsecureOverrideBeforeDiscovery pins it. - - // The data API scopes by slug, so scope and display name coincide. - slug := currentRepoSlug(ctx) - return api.NewClientWithBaseURL(target.Token, target.BaseURL), slug, slug, nil + repoID, repoSlug := currentRepoRef(ctx) + return client, repoID, repoSlug, nil } func handleRecapFetchError(w io.Writer, err error) error { @@ -269,15 +224,7 @@ func terminalWidth(w io.Writer) int { return width } -func currentRepoSlug(ctx context.Context) string { - _, owner, repoName, err := gitremote.ResolveRemoteRepo(ctx, "origin") - if err != nil || owner == "" || repoName == "" { - return "" - } - return owner + "/" + repoName -} - -// currentRepoSlugWithForge is like currentRepoSlug but includes the forge +// currentRepoSlugWithForge returns the origin remote as owner/repo, with the forge // prefix (e.g. "gh/owner/repo", "et/proj/repo") when the remote maps to a // known forge. Code search needs this because the repo index FullName may // include the forge prefix (especially for Entire forge repos stored as diff --git a/cmd/entire/cli/recap_test.go b/cmd/entire/cli/recap_test.go index ab584faffd..c257384254 100644 --- a/cmd/entire/cli/recap_test.go +++ b/cmd/entire/cli/recap_test.go @@ -176,7 +176,7 @@ func TestNewRecapClient_RejectsInsecureOverrideBeforeDiscovery(t *testing.T) { t.Setenv(userdirs.EnvConfigDir, t.TempDir()) t.Setenv(userdirs.EnvCacheHome, t.TempDir()) t.Setenv(api.BaseURLEnvVar, "http://recap.invalid") - t.Cleanup(auth.SetResolveContextForAPIForTest(t, + t.Cleanup(auth.SetResolveContextForCellAPIForTest(t, func(context.Context, string, string, string, *http.Client, clusterdiscovery.DebugFunc) (*contexts.Context, error) { t.Fatal("discovery ran against an insecure override") return nil, errors.New("unreachable") @@ -201,7 +201,7 @@ func TestRunRecap_InsecureOverrideMessageNamesTheVariable(t *testing.T) { t.Setenv(userdirs.EnvConfigDir, t.TempDir()) t.Setenv(userdirs.EnvCacheHome, t.TempDir()) t.Setenv(api.BaseURLEnvVar, "http://recap.invalid") - t.Cleanup(auth.SetResolveContextForAPIForTest(t, + t.Cleanup(auth.SetResolveContextForCellAPIForTest(t, func(context.Context, string, string, string, *http.Client, clusterdiscovery.DebugFunc) (*contexts.Context, error) { t.Fatal("discovery ran against an insecure override") return nil, errors.New("unreachable") @@ -220,6 +220,36 @@ func TestRunRecap_InsecureOverrideMessageNamesTheVariable(t *testing.T) { } } +// With no login, recap prints the login hint itself instead of falling back +// to the data API and rendering that server's 401. +// +// Not parallel: it isolates the user config dir and unsets env overrides. +func TestRunRecap_PrintsLoginHintWhenNotLoggedIn(t *testing.T) { + unsetEnv(t, auth.EnvTokenVar) + unsetEnv(t, api.BaseURLEnvVar) + repoDir := t.TempDir() + testutil.InitRepo(t, repoDir) + t.Chdir(repoDir) + t.Setenv(userdirs.EnvConfigDir, t.TempDir()) + t.Setenv(userdirs.EnvCacheHome, t.TempDir()) + + var out, errOut bytes.Buffer + err := runRecap(t.Context(), &out, &errOut, &recapFlags{}) + if !errors.Is(err, auth.ErrNotLoggedIn) { + t.Fatalf("error = %v, want ErrNotLoggedIn", err) + } + var silent *SilentError + if !errors.As(err, &silent) { + t.Fatalf("error = %T %v, want SilentError", err, err) + } + if out.Len() != 0 { + t.Errorf("stdout = %q, want empty", out.String()) + } + if want := "Not logged in. Run 'entire login' to authenticate."; !strings.Contains(errOut.String(), want) { + t.Errorf("stderr = %q, want %q", errOut.String(), want) + } +} + func TestHandleRecapFetchError_UnauthorizedPromptsLogin(t *testing.T) { t.Parallel() diff --git a/docs/development/api-routing.md b/docs/development/api-routing.md index 0d88217bd6..4ca0d19bd5 100644 --- a/docs/development/api-routing.md +++ b/docs/development/api-routing.md @@ -80,8 +80,10 @@ therefore has exactly three routing shapes, mirroring the entire.io BFF: instead of re-deriving it from a data-plane repo listing. - **User-scoped `/me` → home cell, never fan out**: `auth.NewEntireAPICellClient(ctx, insecure, nil)` routes by the - `home_jurisdiction` JWT claim; activity/recap use it with a data-API - fallback (`runAuthenticatedActivityAPI` in `entireapi_client.go`). + `home_jurisdiction` JWT claim. Activity and recap use it with no data-API + fallback (`runAuthenticatedActivityAPI` in `entireapi_client.go`, + `newRecapClient` in `recap.go`): the BFF serves `/me/*` by proxying to the + same home cell, so it cannot answer a caller the cell path cannot. - **Repo-set queries → fan out and merge client-side**: `cell_fanout.go` — `groupReposByCell` (repo index → per-cell groups; the catalog join key is `ClusterSlug`↔`Cluster.Slug`, NOT the cell name, which the catalog does not