Fix Mirror List Core Request Dialing

Claude Code·Opus 4.8[1m]·

Soph·3mo ago·3hr 14min·5 Checkpoints·8 file changes·+277/-20·296.7K tokens

Code Review — PR #1475: route cluster-addressed mirror commands to the cluster's core

Overview

Fixes mirror mutation verbs (create/remove, collaborators add/remove/list) 400ing with unknown cluster_host when the active context's core doesn't front the target cluster. The insight is sound: these are the CLI's first cluster-anchored control-plane calls — the core to dial is a property of the supplied <cluster-host>, not the active login. The fix routes them through /.well-known/entire-cluster.json discovery, exactly mirroring the established git (NewRepoTokenSource) and data-API resolution paths. Identity-anchored commands (repo create/list, mirror list/get) correctly stay on the active context.

Strengths

Architecturally consistent. ResolveControlPlaneTargetForCluster reuses the same resolveContextForCluster seam, userdirs.Config()/Cache(), and repoExchangeTransportForTest as the repo-token path — no new discovery machinery, just a new entry point. This is the right call.
Clean, behavior-preserving refactors. targetForContext, clientFromEnvToken, clientForTarget, runCoreClient, and renderCoreList are all pure extractions that leave the active-context path identical and isolate the cluster variants to "which core does newClient dial." Verified the non-cluster commands are untouched.
Strong test. TestResolveControlPlaneTargetForCluster_DialsClusterCoreNotActive deliberately seeds a token only for the cluster core, so a regression that dialed the active context fails loudly rather than passing by luck — exactly the right way to test a routing fix. Empty-host guard is covered too.
Documentation is thorough and the NO TRUST GATE reasoning (env-token sent verbatim as bearer vs. exchanged as STS subject_token → no aud-redirection escalation) is correctly carried forward and extended to NewForCluster.

Issues / suggestions

1. mirror list header can lie when ENTIRE_TOKEN is set (low severity). The header calls auth.ResolveControlPlaneTarget(), which ignores ENTIRE_TOKEN and resolves from the active context. But the actual request via coreapi.New() honors ENTIRE_TOKEN and dials the token's aud. If both ENTIRE_TOKEN and a contexts.json are present, the header would name the context core while the request hits a different core.

In the normal CI runner (env token, no contexts.json), ResolveControlPlaneTarget() errors and the best-effort header is silently skipped — so no wrong header in the common case. But the mismatch case is reachable. Consider skipping the header when os.LookupEnv(auth.EnvTokenVar) is set, so the header never contradicts the request:

2. Double resolution for the mirror list header (cosmetic). The target is resolved once for the header and again inside runCoreList → coreapi.New(). Harmless (active-context path is local, no network) but slightly redundant; not worth restructuring unless #1 prompts a touch-up there anyway.

3. No coverage of the discovery-error / login-hint path through NewForCluster. The "no eligible context → login hint" behavior is the user-facing payoff described in the doc comment, but it lives in clusterdiscovery.ResolveContextForCluster (stubbed in tests). Assuming that's tested in clusterdiscovery, this is fine — just noting the new wrapper only has the happy path and empty-host covered.

Risk

Localized to cluster-scoped mirror commands; the active-context and env-token paths are provably unchanged by the refactor. The main residual risk is mis-discovery sending a mutation to the wrong federation, but that's the same trust path git/data-API already rely on. Cursor's "Medium Risk" framing is fair.

Verdict: approve. The fix is correct and well-tested. Issue #1 is the only one I'd consider addressing before merge (a one-line guard); #2 and #3 are optional.

Want me to run mise run lint && mise run test to confirm the tree is green, or push a fix for the ENTIRE_TOKEN header edge case?

Done. I created a new branch stacked on the PR branch and committed the fix for issue #1.

Summary

Branch: fix/mirror-list-header-env-token (off fix/control-plane-cluster-routing, since the header code only exists on the PR branch — not yet on main)

The fix: The mirror list stderr banner called auth.ResolveControlPlaneTarget(), which ignores ENTIRE_TOKEN. In env-token mode the request actually dials the token's own aud (via coreapi.New → clientFromEnvToken), so when both ENTIRE_TOKEN and a contexts.json were present, the banner named a core the request never talks to. Now the banner is skipped whenever ENTIRE_TOKEN is set.

What changed:

Extracted the inline banner logic into a pure, testable mirrorListHeader(json bool) string helper that returns "" for the three skip cases (--json, env-token set, no resolvable context) and the "Listing mirrors on <core>" string otherwise.
Added TestMirrorListHeader covering all four cases — the key one asserts the banner is suppressed even when a resolvable active context exists, proving the env-token guard wins.

Verification: mise run fmt + mise run lint (0 issues) + full cmd/entire/cli package tests pass.

Note this is stacked on the unmerged PR #1475, so it should either merge after #1475 lands or be folded into it. Want me to push the branch and open a PR (targeting fix/control-plane-cluster-routing), or leave it local?

Done. I reworked the fix into a structural one rather than the inline env-token check.

What changed and why

Your instinct was right — but the answer turned out to be the opposite of what you remembered. I traced every ENTIRE_TOKEN site (via a search agent + direct reads):

There is no shared "effective core" helper. The env-token → active-context precedence lives in exactly one place: coreapi.New(). auth.ResolveControlPlaneTarget() only knows the active-context half and ignores ENTIRE_TOKEN entirely.
auth status does not consult ENTIRE_TOKEN either — resolveStatusTarget (auth.go:205) reads purely from the active context. So it has the same latent bug: in env-token mode it would report the wrong core. (Pre-existing; see below.)
coreapi.Client hid its serverURL with no getter, which is why every "talking to X" display site was forced to re-derive the core and could get it wrong.

So instead of replicating the env-token check (my first attempt), the root-cause fix is to stop re-deriving and ask the client what it dials:

Added coreapi.Client.CoreOrigin() — single source of truth, reports whatever was actually wired in (active context, NewForCluster's cluster core, or the ENTIRE_TOKEN aud).
The mirror list banner now renders from the live client's CoreOrigin(), so it can't diverge from where the request goes — and it's now correct in env-token mode (names the token's core) rather than just suppressed.
Tests moved to where the logic now lives: TestClient_CoreOrigin (the getter) and TestNew_CoreOrigin_HonoursEnvToken (proves the env-token path reports the token's aud).

Verified: mise run fmt + lint (0 issues) + coreapi and full cmd/entire/cli package tests pass. Commit amended on fix/mirror-list-header-env-token.

Two follow-up thoughts

auth status has the same divergence (shows active-context core, ignores ENTIRE_TOKEN). It's pre-existing and out of scope for this branch, but now that CoreOrigin() exists it'd be a clean fix. Want me to do it as a separate change?
Broader prevention: CoreOrigin() is now the tool any future display site should use. Worth a one-line note in the coreapi doc or CLAUDE.md ("to show the user which core, ask the client, never re-resolve") if you want to make the convention explicit.

Should I push the branch / open the PR (targeting fix/control-plane-cluster-routing), and do you want the auth status follow-up?

Both follow-ups are done and committed on fix/mirror-list-header-env-token. Full unit suite (6551 tests), lint (0 issues), and fmt all clean.

What landed

1. entire auth status now honors ENTIRE_TOKEN (auth.go)

New resolveAuthStatusTarget branches on ENTIRE_TOKEN before falling back to the active context, mirroring the precedence coreapi.New uses.
New resolveEnvTokenStatusTarget derives the core from the token's aud (via the same CoreURLFromEnvToken the client uses) and sends the token verbatim as the bearer — fail-closed on blank/malformed, same as coreapi.New.
runAuthStatus renders env-token mode cleanly: names the core + "ENTIRE_TOKEN environment variable" bearer, and skips the context/keychain/session lines (you can't manage an env-token session).
logout is deliberately left on the active context — it manages a stored login session, which an ephemeral env var doesn't have. I scoped the env-token logic to the status path rather than the shared resolveStatusTarget so logout semantics are unchanged.
Tests: TestRunAuthStatus_EnvTokenMode (render) and TestResolveEnvTokenStatusTarget (valid aud / blank / no-URL-aud).

2. Convention documented (CLAUDE.md → new "Control-Plane Core Resolution" pattern)

The rule: to display which core a request uses, ask the client (CoreOrigin()); never re-resolve with ResolveControlPlaneTarget for display, because it silently ignores ENTIRE_TOKEN and the cluster case.
Notes the one exception (commands like auth status that build a client outside coreapi must apply the env-token-first precedence themselves) and why logout differs. AGENTS.md symlinks to CLAUDE.md, so it's covered too.

The whole branch is now one commit. Want me to push it and open the PR against fix/control-plane-cluster-routing?

/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. Also flag long-lived objects built from closures or captured environments — they keep the entire enclosing scope alive for the object's lifetime (a memory leak when that scope holds large values); prefer a class/struct that copies only the fields it needs. 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·1m

Done. The four cleanup agents ran in parallel and I applied the one actionable finding.

Applied

Reuse — extracted auth.ParseEnvToken. The TrimSpace → blank-check → CoreURLFromEnvToken sequence was duplicated inline in both coreapi.clientFromEnvToken and auth.resolveEnvTokenStatusTarget. Pulled it into a single auth.ParseEnvToken(raw) (coreURL, token, err) in env_token.go — now the one owner of that fail-closed validation, so future tightening can't drift between the two call sites. Both callers were simplified to delegate to it, and I added a direct TestParseEnvToken (trims + returns verbatim token / blank / no-aud).

Skipped (with reasons)

Simplification — clientFromEnvToken's (client, bool, error) triple-return. The agent itself rated this low-priority and "idiomatic but awkward." It's pre-existing code from PR #1475 (not part of my changes), the (value, ok, err) shape is a normal Go idiom, and a sentinel-type/named-error rewrite trades one form of indirection for another without a clear win. Not worth the churn.
Efficiency — none. The agent confirmed the new code removes a double-resolution (the old banner resolved the target separately from the request); nothing wasteful introduced.
Altitude — none. Confirmed CoreOrigin() is the right depth (single source of truth), the runCore*/renderCoreList wrapper split is justified via an injectable client-factory, and the auth status precedence handling is structurally separate (it needs the (coreURL, token) pair for ListAuthSessions) rather than a shallow special-case — which is exactly why the small ParseEnvToken extraction is the correct granularity, not merging the whole paths.

Verification: mise run fmt, mise run lint (0 issues), and the auth / coreapi / cli package tests all pass. Commit amended on fix/mirror-list-header-env-token.

Done. PR #1478 is open against main.

Summary

Confirmed #1475 merged into origin/main, then rebased my work onto the updated main — the diff is now exactly my 8 files / one commit, with no leftover control-plane commits.
Renamed the branch to fix/display-effective-core (the old fix/mirror-list-header-env-token understated the scope, which now spans mirror list + auth status + the CoreOrigin mechanism).
Lint clean before push (per the repo's pre-push rule).
Pushed and opened PR #1478 against main with a full write-up.

The PR bundles everything from this session: the CoreOrigin() single-source-of-truth fix, the auth status env-token correctness fix, the auth.ParseEnvToken dedup from /simplify, and the CLAUDE.md convention note.