Centralize API Authentication Handling

Yes — based on the current code, the behavior you saw is a bug / UX omission, not expected behavior.

What should happen now:

• entire trail list calls runTrailListAll
• that calls NewAuthenticatedAPIClient
• if there’s no valid login, it should fail before hitting the trail list API with something like:

Relevant code:

• cmd/entire/cli/trail_cmd.go
  • runTrailListAll(...)
  • creates client via NewAuthenticatedAPIClient(...)
• cmd/entire/cli/api_client.go
  • NewAuthenticatedAPIClient(...)
  • maps missing auth to not logged in (run 'entire login' first)

So if nightly printed:

while logged out, then that version likely allowed an unauthenticated API request and treated the empty/anonymous response as a real empty trail list. That’s misleading.

I’d classify it as:

Bug: auth-required data was rendered as an empty successful result instead of prompting login.

I did notice similar-but-not-identical patterns elsewhere:

• activity handles auth explicitly and prints:
• search resolves auth up front and returns:
• recap is intentionally different: it may call the API without a token so the server returns 401, then renders a login-oriented message. That’s documented in code.
• shell completion intentionally swallows auth errors, which is fine because completions shouldn’t spam the terminal.

So trail list should align with activity/search, not silently show an empty state. Current repo code appears to have already moved in that direction; nightly may be behind or from a code path/version before this was fixed.

Yes. I’d dedupe this with one shared “authenticated command runner” for data-API commands.

Right now there are two patterns:

1. Core/control-plane commands already deduped:

   • runCore(...)
   • runCoreJSON(...)
   • used by org, repo, project, grant
   • centralizes auth/client/error rendering.

2. Data API commands are ad hoc:

   • trail
   • activity
   • search
   • dispatch
   • setup
   • recap is intentionally special

That second group is where bugs like this can slip in.

I’d add something like:

And one shared renderer:

Then trail list becomes structurally unable to skip auth:

Key refactor: make lower-level functions accept *api.Client instead of constructing auth internally.

Example:

Commands that should use this shared path:

• trail list/show/create/update/review/watch
• activity
• dispatch cloud paths
• maybe setup API calls
• search, though it currently has custom search-service token resolution

Commands that should not use it:

• login
• logout
• auth status
• recap, because code explicitly says it may call without a token to let the server return 401 and render a recap-specific message
• shell completions, because they intentionally swallow auth errors

So: yes, we should centralize auth for data-API commands just like runCore already does for core commands. That would make “auth required” a wrapper-level property instead of something each command remembers manually.

Implemented.

Changed:

• Added shared auth gate:
  • cmd/entire/cli/authenticated_data_api.go
  • runAuthenticatedDataAPI(...)
  • shared handling for:
    • not logged in → Not logged in. Run 'entire login' to authenticate.
    • Ctrl-C/context cancellation → silent error
    • other auth/token/network errors → surfaced as-is

Updated callers:

• activity
• trail
  • show
  • list
  • create API phase
  • update
  • review fallback list
  • review target auth
  • watch

Added regression test:

• TestRunTrailListAll_PrintsLoginHintWhenNotLoggedIn
• verifies logged-out trail list does not print No trails found.

Verified:

Passed.

Mostly yes, but with one caveat.

It matches the existing core command pattern conceptually:

• existing: corecmd.go
  • runCore(...)
  • builds authenticated core client
  • centralizes auth-ish setup/error handling
• new: authenticated_data_api.go
  • builds authenticated data API client
  • centralizes data API auth error handling

So architecturally it’s aligned.

Caveat: the file name/style is a little more descriptive than the existing naming. If we want it to feel more native to the repo, I’d rename it to something like:

or

But function-wise, yes: it follows the existing “shared command wrapper” pattern already used by runCore / runCoreJSON.

Renamed it to:

Re-tested targeted auth tests: passed.

Why not use corecmd.go directly?

• corecmd.go is specifically for the control-plane/Core API:

  • uses internal/coreapi.Client
  • calls coreapi.New()
  • renders core API problem-detail errors
  • has JSON helpers for core resources

• Trail/activity use the data API:

  • uses cmd/entire/cli/api.Client
  • calls NewAuthenticatedAPIClient
  • token is scoped to api.BaseURL()

So the pattern is the same, but the client/auth target/error types are different enough that sharing runCore itself would make it muddy. Keeping a sibling wrapper in authcmd.go is cleaner.

Before:

1. Each command created auth/API clients itself, e.g. trail list called NewAuthenticatedAPIClient(...) inside its own flow.
2. Each command decided separately how to handle auth errors, so some paths could accidentally render normal output like No trails found instead of a login prompt.

After:

1. Auth-required data API commands enter through runAuthenticatedDataAPI(...), which creates the client first and handles login/cancel errors consistently.
2. Command-specific logic only runs after auth succeeds, so trail list cannot reach the empty-state rendering while logged out.

Affected by this change:

• entire activity
• entire trail
• entire trail list
• entire trail create
• entire trail update
• entire trail watch
• entire trail review ... paths that resolve/watch/list trails

Not changed:

• entire search — uses its own search-service token flow.
• entire recap — intentionally allows anonymous request first, then handles 401.
• entire setup/enable report call — best-effort background-ish reporting, intentionally skipped if auth fails.
• dispatch wizard repo discovery — still has its own injection/test seam.

Yes.

From this repo:

If you want to ensure you’re testing this build, not nightly:

Good checks:

Expected while logged in: activity UI/static output loads.

Logged-out check without touching your real login is harder because auth uses your normal keyring/context. Safer option:

Expected:

Committed and pushed.

Commit:

Verification before commit/push:

Passed.

You can test this safely without logging out for real by isolating Entire’s auth/config to a temp dir/file.

From repo root:

Test logged-out trail list

Run from a git repo with an origin, e.g. entire.io:

Expected:

And importantly, not:

Test logged-out activity

Expected same:

iTerm2 vs macOS Terminal shouldn’t matter here. The temp env vars are the important part.

Yeah, good catch — that command tests a different logged-out shape.

Your command isolates both:

So the CLI has no login context at all. It fails before token lookup:

That is not the same as “known auth context, but no usable token”.

To test the case we changed, keep your real config/context, but isolate only the token store:

And for the local build:

Same for activity:

Expected after our change:

Important bit: don’t set ENTIRE_CONFIG_DIR for this test, because then you’re testing “no configured auth context”, not “not logged in”.

That happened because:

creates an empty file. The file token store then tries to parse that empty file as JSON and errors with:

Use a path that does not exist yet instead:

And:

Expected with our local/dev build:

If testing nightly, it may still show the old/raw form:

That’s fine — the thing you’re checking is whether it incorrectly says:

It should not.

Right — that test only covers “no token available”. It won’t reproduce the original bad output.

Your original bug likely required a bad/expired/mismatched token that still let the CLI make a request, then the server responded with an empty/unauthorized-ish trail result that the CLI rendered as:

The isolated token-store test now stops earlier, so it verifies our new guard, not the old exact bug.

Practical verification now:

Expected/pass:

And:

Expected/pass:

So you’re validating the fix’s invariant:

unauthenticated data-API commands never reach command-specific rendering like No trails found.

To reproduce the exact old nightly behavior, we’d need the same auth state you had then: probably stale context/token combo, not simply “no token”.

I tried to reproduce it locally and could not get No trails found from a logged-out state.

What I tested:

Got:

Got on installed nightly:

Got on our local build:

Also tried bogus token:

So I can verify the new behavior, but I can’t reproduce your exact original No trails found with the currently installed nightly.

Most likely explanations:

1. Your original run had a different auth state than these synthetic cases.
2. The nightly you ran then was different from the currently installed nightly.
3. The server/API behavior changed between then and now.

The key thing our fix guarantees: command rendering like No trails found is now unreachable unless auth client creation succeeds first.

Created PR:

Failing tests first:

• CI failure was in unrelated test:
  • internal/remotehelper/transport/proxy_test.go:974
  • TestColdPathRedirectAllReplicasFail
• Cause: flaky test used closed localhost ports as “dead” replicas. In parallel CI, a port can get reused, so the test sometimes got plain server error (HTTP 500) instead of the expected failover error.
• Fixed by using deterministic httptest.Servers that return 503.
• Verified:
  • go test ./internal/remotehelper/transport -run TestColdPathRedirectAllReplicasFail -count=20
  • go test ./cmd/entire/cli
  • mise run fmt && mise run lint
• Pushed commit:
  • 78d227902 Stabilize redirect failover test

Copilot comment:

runTrailListAll now authenticates before validating CLI inputs...

Yes, the concern is valid.

Example currently:

Because auth now wraps the whole command, a logged-out user gets:

instead of the local validation error:

So Copilot is right: cheap local validation (--limit, --status) should ideally happen before auth.

Severity: low/medium UX issue, not a security bug. The fix direction would be to split validation out and run it before runAuthenticatedDataAPI, while still keeping API work behind the auth wrapper. I have not changed this yet.

No — “flaky” is plausible, but I’m not sure.

What I found:

• The failed PR run had:
• I searched recent failed runs and did not find another occurrence of:
  • TestColdPathRedirectAllReplicasFail
  • all 2 nodes failed
  • that same server error (HTTP 500) mismatch

So evidence says: single observed occurrence recently, not proven flaky.

Why I suspected flake:

• The test used closedServerURL(): start an httptest.Server, grab its 127.0.0.1:port, close it, then assume that port stays dead.
• That assumption can be invalid if something reuses the port.
• The failure shape fits “we reached an HTTP server unexpectedly” better than “our auth PR broke transport”.

But: I can’t prove port reuse from logs. A more careful phrasing is:

The failure appears unrelated to this PR and was likely caused by a brittle test assumption around closed localhost ports. I found no recent repeats, so it’s not established as a known flaky test.

Done. Removed the test stabilization commit locally.

Current HEAD:

No push performed.

Two options:

Option 1 — Minimal fix

Validate trail list inputs before auth.

Example shape:

Pros:

• Smallest change.
• Directly addresses Copilot.
• Keeps auth wrapper behavior for real API work.

Cons:

• Only fixes trail list; other commands may still auth before local validation.

Option 2 — Broader pattern

Add a standard “validate before auth” convention for wrapped data API commands.

Example shape:

Pros:

• Prevents this class of bug across future commands.
• Makes the intended ordering obvious.

Cons:

• More abstraction.
• Slightly heavier than needed for this PR.

My pick: Option 1. It’s enough for the actual comment, low-risk, and avoids adding framework until we see this pattern repeat.

Implemented Option 1 locally.

What changed:

• trail list now validates options before auth.
• Added regression test for --limit 0 not triggering the login hint.

Verified:

Committed locally:

Not pushed yet. Note: pushing this PR branch will need a force push because we removed the previous test-stabilization commit from local history.

Yes — good catch. I had duplicated the Limit <= 0 validation.

I fixed it by removing the old inline limit check from runTrailListAllWithClient; validation now lives in:

and runs before auth in runTrailListAll.

I amended the commit:

Verified again: