Isolate Token Store in CLI Tests

Both research agents finished — I now have the full inventory. Before I re-pose anything: you flagged that you'd like to clarify the questions. What would you like to clarify or add?

For context, here's what the research established (grounding the plan):

• 26 production NewGitStore(...) sites across cli, strategy, dispatch, benchutil, plus the in-package one in checkpoint.LookupSessionLog (committed.go:1338) — exactly the spread the issue predicted. (Plus ~131 test-file calls.)
• No checkpoint.Open, CheckpointStores, or OpenOptions exists yet — all net-new.
• The facade must carry: BlobFetcher (wired at 3 sites: resume.go:191, explain.go:861, manual_commit.go:47), and non-read/write accessors store.Refs() (6 sites) and store.Repository() (explain.go:936) so nothing type-asserts back to *GitStore.
• attach is the key exception: it injects settings via committedRefs() and uses refs.PrimaryAsRead() at attach.go:366,466 — Phase 0 must preserve that exact topology through OpenOptions.Settings/Refs, not regress to live-settings resolution.

The two open decisions I was going to ask about:

1. Scope — you indicated Phase 0 only (matches the issue's "land Phase 0 first").
2. Shared instance (pjbgf's point 1b) — route every site through Open only, vs. also sharing one CLI-lifecycle instance. My lean is route-through-Open-only for a smaller, no-behavior-change diff, with attach keeping its own instance regardless.

What's on your mind?

Honestly, for this codebase the benefit is modest. Breaking it down concretely:

GitStore is a thin wrapper — {repo, refs, blobFetcher}. Construction itself (NewGitStore) is just a struct alloc; it's free. The cost is in the two things each site re-does around it:

1. gitrepo.OpenCurrent(ctx) — opens the repo, reads git config (and goes through that symlink-following configloader). Filesystem work.
2. ResolveCommittedRefs(ctx) — reads .entire/settings.json from disk to resolve the topology.

So a shared instance buys you:

• Fewer redundant repo-opens and settings-reads. Real, but marginal — a single CLI invocation runs one command, so only a handful of those 26 sites actually execute per run. This isn't a hot loop.
• Consistency / one source of truth. Every consumer sees the same resolved refs and the same blobFetcher. This is the more interesting win: today the blobFetcher has to be manually re-attached (withBlobFetcher, the three scattered SetBlobFetcher calls) precisely because instances are built independently and some forget the fetcher. A single instance closes that footgun.

pjbgf's own framing ("short-lived anyway") cuts both ways — the lifecycle is short enough that the perf win is small, and short enough that sharing is safe.

Here's the key point though: routing everything through checkpoint.Open already delivers the consistency win (one place that resolves refs + attaches the fetcher), without needing to literally memoize and thread a singleton through call sites. The singleton only adds the (marginal) perf saving on top, at the cost of plumbing a shared handle everywhere and reasoning about whether sharing a *git.Repository across the call graph is safe.

So my recommendation: do Open now, skip the literal shared singleton. It captures ~all the real benefit, keeps Phase 0 a clean no-behavior-change mechanical diff (which is the whole point of landing it first), and the singleton can be layered on trivially later if profiling ever shows repo-opens mattering. attach stays the documented exception either way since it needs PrimaryAsRead().

Want me to write up the Phase 0 plan on that basis?

Confirmed — and importantly, the keychain prompt is not from my changes. Here's the diagnosis:

The cli package's TestMain (global_test.go:20) only mocks the zalando keyring (keyring.MockInit()), which is in-process. The keychain pop comes from a cli test that spawns a subprocess (the real entire binary or a git-hook that re-invokes it) — and testing.Testing() is false in that child, so it reaches the tokenstore default backend = real OS keychain. Forcing ENTIRE_TOKEN_STORE=file made it inherit isolation and the prompt disappeared (the run above passed clean).

None of my Phase 0 edits touch auth/token/credential code — they're purely checkpoint-store construction (Open → ResolveCommittedRefs → reads the settings file, never the keychain). So this is a pre-existing isolation gap, surfaced because you ran go test ./cmd/entire/cli/ directly. It'd be a good separate fix to have that package's TestMain set the file-backend env vars process-wide (like the integration/e2e TestMains do), but it's orthogonal to this work.

Phase 0 is implemented — summary

New seam (cmd/entire/cli/checkpoint/open.go): Open(ctx, repo, OpenOptions) (*Stores, error) — the single construction point, with OpenOptions{BlobFetcher, Settings, Refs} and the facade Stores{Primary} plus accessors Temporary(), Refs(), Repository().

Migrated all 26 production NewGitStore(...) sites through Open, across cli (resume, attach, explain, rewind, attribution, review_context, head_checkpoint_flags), dispatch, strategy (the getCheckpointStore seam + cleanup/common/hooks/rewind/condensation), and the in-package checkpoint.LookupSessionLog. Accessor sites (Refs(), Repository()) and temporary-method sites now route through the facade so Phase 1 changes internals only.

Verification: go build ./... ✓, mise run lint → 0 issues, checkpoint/strategy/dispatch tests ✓, full cli package ✓.

Decisions worth flagging (deviations from the issue's sketch):

1. Type is checkpoint.Stores, not CheckpointStores — revive flags the latter as a stutter; checkpoint.Stores is idiomatic and reads cleanly (stores.Primary). Trivial to rename if you want it to match the issue verbatim.
2. getCheckpointStore now returns (*GitStore, error) (propagated through its 7 callers); folded the old withBlobFetcher into OpenOptions.BlobFetcher.
3. generateCheckpointSummary now takes the facade (it needs both committed-write and Repository() — clean migrate-once).
4. Open's always-nil error has a //nolint:unparam explaining it's the forward-looking contract per the issue.
5. No-behavior-change preserved: I initially had the summary mirror read stores.Refs(), which broke REDACTED (the test opens its store with DefaultV1Refs but opts into the mirror via settings). Reverted that one line back to ResolveCommittedRefs(ctx).
6. Left the ~131 test-file sites and benchutil on NewGitStore (test infra; NewGitStore is retained as Open's internal constructor).

I have not run test:ci/integration/e2e (heavier, and I don't want to re-trigger that keychain path without isolation). Want me to (a) commit this on a branch, (b) also patch the cli TestMain keychain-isolation gap, or (c) keep going into Phase 1 (interface split)?

Both branches are committed. Done.

Summary

Branch 1 — soph/pluggable-stores-phase0 (commit 18b938d6a) Phase 0 of issue #1433: centralized checkpoint store construction behind checkpoint.Open(ctx, repo, OpenOptions) (*Stores, error). Migrated all 26 production NewGitStore(...) sites; facade exposes Primary/Temporary()/Refs()/Repository() so Phase 1 only changes internals. Build ✓, lint 0 issues, checkpoint/strategy/dispatch/cli tests ✓.

Branch 2 — soph/cli-testmain-token-isolation (commit 6a2ed442d, off main) Fixes the keychain prompt you hit. The cli TestMain only mocked the in-process zalando keyring; tests that spawn the real entire binary (or a git hook) run in a child where testing.Testing() is false, so the child's tokenstore default backend reached your real OS keychain. Now TestMain sets ENTIRE_TOKEN_STORE=file (+ isolated token/auth/config/cache paths) process-wide before m.Run, so children inherit file-backed isolation. Verified by running go test ./cmd/entire/cli/ with the external env vars explicitly unset — passes, no keychain prompt. Lint clean.

Both branches are independent of each other and of main. The unrelated untracked dirs (.antigravitycli/, .gstack/) were left alone.

Two things I did not do (let me know if you want them):

• Push either branch / open PRs.
• Run the heavier test:ci (integration + e2e canary) on the Phase 0 branch.

Note: the Phase 0 branch reproduces the same keychain gap if you run its full cli test suite — the two fixes are on separate branches, so you may eventually want branch 2 (or its equivalent) to land first, or to rebase Phase 0 on top of it.