Entire

Bottom line

The PR is sound and does the right thing for the data & core first-class paths — both now resolve through the context machinery and refresh JWTs when a refresh token exists. The discovery refactor is a clean consolidation. But "all API calls refresh if possible" is not literally true: three paths still don't refresh, and one of them (RepoScopedToken) carries a now-stale justification comment. Two of the three are explicitly deferred to COR-395, so they're acceptable for this slice — but you should know they're open.

Path-by-path refresh audit

Call site	Resolves via	Refreshes?	Verdict
Core API `coreapi.New` (status/dispatch/activity), active context	`ResolveControlPlaneTarget` → `NewRefreshingLoginProvider`	✅	correct, keyed on `c.CoreURL`
Core API, no active context	`staticControlPlaneTarget` → `TokenForResource` → `defaultManager` (no `RefreshPath`)	❌	OK — only legacy/no-refresh-token logins land here
Data API: `api_client`, `recap`, `search`, `dispatch/mode_cloud`	`ResolveDataAPIToken` → discovery → `NewRefreshingResourceProvider`	✅	correct
Data API: discovery-unavailable fallback	`TokenForResource` → `defaultManager`	❌	see Finding 2
`auth status` / `logout`, active context	`RefreshedLoginToken` → `NewRefreshingLoginProvider`	✅	correct, with raw-token fallback
`RepoScopedToken` (mirror probe)	direct `LookupCurrentToken` + `sts.Client`	❌	see Finding 1 (COR-395)
`resolveAuthHostToken` (auth.go:83)	`TokenForResource`	❌	dead — tests only (COR-395)

All four data-API call sites and the live control-plane path are correctly on contexts+refresh. Build green, affected package tests green.

Findings (ranked)

1. RepoScopedToken doesn't refresh, and its comment is now wrong. COR-389's acceptance #2 explicitly names it. The comment in repo_token.go:60-74 justifies the skip with "entire login (device flow) stores only a bare access token — no refresh token." That premise is stale: login now requests Scope: "cli offline_access" (auth/client.go:72) and contexts.go:84 persists the refresh token. So an expired-but-refreshable login will fail here. This is real, but COR-395 ("RepoScopedToken bypasses cluster discovery… + remove dead resolveAuthHostToken") tracks it — so deferring is defensible. Fix the comment regardless, or it will mislead the next reader.

2. Data-API discovery-unavailable fallback silently won't refresh AND retains the multi-core bug. When /.well-known/entire-api.json is absent, ResolveDataAPIToken drops to TokenForResource → singleton defaultManager, keyed on api.AuthBaseURL(). If the active context lives on a different core, the exchange targets the wrong core with a token minted elsewhere — the exact bug COR-389 calls out — and won't refresh even though the active context has a refresh token. Only hit on un-rolled-out deployments, justified as "never worse than before," but it's a latent inconsistency. The cleaner design (see below) would eliminate it.

3. Legacy-bridge asymmetry. ResolveDataAPIToken calls MigrateLegacyLoginContext(); ResolveControlPlaneTarget does not. A legacy-only login therefore gets bridged-and-refreshed on a data command but falls to the non-refreshing static path on a control-plane command. Practical impact ≈ nil (legacy logins predate offline_access), but the two entry points should behave the same. One-liner to add the bridge call to ResolveControlPlaneTarget, or a note saying why not.

Simplification assessment

Mostly yes — the refactor is genuinely simpler: fetchWellKnownJSON, selectContext(subject, …), and renderLoginHint(subject, …) are well-extracted; sharing the ClusterCoresCache machinery (separate file to avoid host-name collisions) is the right call. No redundant abstractions stood out. Minor nits:

resolveAuthHostToken is dead production code (only auth_test.go calls it). You could delete it now instead of waiting for COR-395 — that's strictly less code in this PR.
The two token stacks still coexist. defaultManager+NewContextStore (read-only, no refresh) survives purely as the fallback for findings 2 & the no-context control-plane case. The end-state COR-389 wants is retiring it. The natural simplification: route both fallbacks through NewRefreshingResourceProvider/NewRefreshingLoginProvider on the active context (try active context first, singleton only when no context exists). That would refresh, fix the multi-core mismatch in finding 2, and let the singleton shrink. But that's the larger COR-389 consolidation — reasonable to keep out of this data-API slice.

Net: ship-able as the data-API slice. Before merge I'd (a) fix the stale RepoScopedToken comment, and (b) either delete resolveAuthHostToken or drop a // TODO(COR-395). Findings 2 and 3 are fine to leave as tracked follow-ups, but call them out in the PR description so they're not mistaken for "done."

Want me to apply the two cheap fixes (comment + dead-code removal) or open them as COR-395 notes?

Done both.

1. RepoScopedToken comment (auth/repo_token.go) — rewritten to state plainly that it does not refresh, that the old "device flow = no refresh token" premise is now false (login requests offline_access and persists a refresh token), and that it's slated for removal/rework in COR-395 alongside the dead resolveAuthHostToken. Package builds clean.

2. PR #1377 body — added a "Known follow-ups (not addressed here)" section covering finding 2 (discovery-unavailable fallback: no refresh + multi-core-naive) and finding 3 (legacy-bridge asymmetry between data and control-plane resolution), with a pointer that RepoScopedToken/resolveAuthHostToken are COR-395.

Note: the auth_test.go:304 undefined: resolveAuthHostToken diagnostic is from your concurrent edit — left untouched.

Committed 8362358e64. Not pushed.

What landed:

Removed dead resolveAuthHostToken (+ its dedicated tests) — your change, completed.
Build fix: your deletion also took out shared test helpers (authMemStore/saveCoreToken/newResolveTestManager) that activity_cmd_test.go depends on — restored those so the package compiles.
RepoScopedToken comment rewritten: states it doesn't refresh, the old "no refresh token" premise is stale, slated for COR-395.

fmt + lint clean, affected tests green. The PR body already carries the findings-2/3 follow-ups from earlier.

/simplify → 4 cleanup agents in parallel → apply the fixes

You are improving the quality of the changed code, not hunting for bugs. Review it for reuse, simplification, efficiency, and altitude issues, then fix what you find. Do not look for correctness bugs — that is what /code-review is for.

Phase 0 — Gather the diff

Run git diff @{upstream}...HEAD (or git diff main...HEAD / git diff HEAD~1 if there's no upstream) to get the unified diff under review. If there are uncommitted changes, or the range diff is empty, also run git diff HEAD and include the working-tree changes in scope — the review often runs before the commit. If a PR number, branch name, or file path was passed as an argument, review that target instead. Treat this diff as the review scope.

Phase 1 — Review (4 cleanup agents in parallel)

Launch 4 independent review agents via the Agent tool, all in a single message so they run concurrently. Pass each agent the diff and one of the four angles below. Each returns its findings with file, line, a one-line summary, and the concrete cost (what is duplicated, wasted, or harder to maintain).

Reuse

Flag new code that re-implements something the codebase already has — Grep shared/utility modules and files adjacent to the change, and name the existing helper to call instead.

Simplification

Flag unnecessary complexity the diff adds: redundant or derivable state, copy-paste with slight variation, deep nesting, dead code left behind. Name the simpler form that does the same job.

Efficiency

Flag wasted work the diff introduces: redundant computation or repeated I/O, independent operations run sequentially, blocking work added to startup or hot paths. Name the cheaper alternative.

Altitude

Check that each change is implemented at the right depth, not as a fragile bandaid. Special cases layered on shared infrastructure are a sign the fix isn't deep enough — prefer generalizing the underlying mechanism over adding special cases.

Phase 2 — Apply the fixes

Wait for all four agents to complete, dedup findings that point at the same line or mechanism, and fix each remaining one directly. Skip any finding whose fix would change intended behavior, require changes well outside the reviewed diff, or that you judge to be a false positive — note the skip rather than arguing with it. Finish with a brief summary of what was fixed and what was skipped (or confirm the code was already clean).

3mo ago·6m

Build, tests, and lint all clean. Here's the summary.

Fixed

Extracted shared resolveCachedCores helper (internal/entireclient/clusterdiscovery/resolve.go). resolveAPICores (38 lines) was a near-verbatim copy of resolveClusterCores — identical cache-load → freshness-check → stale-fallback → live-discover → cache-write logic, differing only in which cache file, discover fn, and debug wording. Both now delegate to one parameterized helper (load/modify/discover funcs + a label). Net ~50 lines of duplication removed; the "Mirrors resolveClusterCores exactly" comment liability is gone. Flagged by both the Simplification and Altitude agents as the top, in-scope-cheap finding.

Skipped (with reason)

Reuse / Efficiency agents: found nothing actionable — code already reuses api.OriginOnly, selectContext, isLoopbackHTTP, etc.
Double URL parse (OriginOnly then hostOf in data_api.go) — skipped. The cheaper form means changing api.OriginOnly's signature (outside the diff) or re-implementing its normalization inline (reuse-negative). It's one extra parse per command, not a hot path.
Unconditional STSPath in newContextTokenManager — skipped. The code comment deliberately keeps a single config to prevent the two providers drifting; splitting it is arguably worse.
selectContext subject param formatting — skipped. Agent rated it "already readable"; the per-call-site "cluster "+host / "API host "+host is clear and consistent.
Two coexisting token-manager stacks (singleton vs per-context) — legitimately deferred to COR-389/COR-395 consolidation, per the agents and the PR's stated scope.

Changes are unstaged (working tree). Want me to commit?

Fixed and committed (2ec7df54f9, not pushed).

Root cause: your commit d471460 ("squash away info about different login servers until ready") stripped the login-server list and entire auth use hint out of renderLoginHint, but two tests still asserted the old richer message → test-core failed (and test just gated on it).

Fix:

TestRenderLoginHint — now asserts the new message + that the URLs/entire auth use are intentionally absent.
TestResolve_NoEligibleContextReturnsLoginHint — dropped the URL + auth-use assertions, same NotContains guard.
Refreshed the stale RenderLoginHint doc comment (still claimed "one indented URL per line").

Verified: full unit suite (6145 tests) passes, fmt + lint clean. Push when ready.