Fix Test Fixture Leaking Into User Config

I just noticed my real, live, auth context list contains "example.com":
09:14:54 (git) paul/cor-393-de...▲ $ entire auth contexts CONTEXT HANDLE LOGIN SERVER
- eu.auth.partial.to toothbrush https://eu.auth.partial.to github:toothbrush@localhost:8180 toothbrush https://localhost:8180 core.example.com bob https://core.example.com
make sure our tests are using a contexts.json (and other config files) safely tucked away in 'mktemp -d' somewhere. DO NOT mess with the user's real life config. they might be using the tool while the tests are running.
The full CI suite (mise run test:ci — unit + integration + Vogon canary) is running in the background; I'll report when it finishes. The change so far, for context:
internal/testdirs(new) — undergo test, config/cache/tokenstore resolution falls back to a throwaway per-process temp dir instead of your real home, so an unisolated test physically cannot touch~/.config/entire,~/.cache/entire, or your Keychain.- Wired into all four surfaces —
contexts.DefaultConfigDir(),discovery.DefaultCacheDir(), versioncheck (which now also honorsENTIRE_CONFIG_DIR— it previously only usedHOME), and the tokenstore default backend (which previously defaulted to your real macOS Keychain in any test package that forgot to swap it). - Subprocess isolation — integration and e2e TestMains now export
ENTIRE_CONFIG_DIR,XDG_CACHE_HOME, and file-backed token-store env vars, since spawnedentirebinaries can't detect they're under test. This was the confirmed gap: both harnesses passed your real environment through.
Done — committed as 4801b30bba9 (not pushed). Tests can no longer touch your real config, cache, or Keychain, in-process or via spawned binaries.
How the leak was possible:
- Unit tests were isolated only by convention — every test had to remember
t.Setenv("ENTIRE_CONFIG_DIR", ...). One forgotten line meant writes to your real~/.config/entire/contexts.json. Thecore.example.com/bobfixture matchescmd/entire/cli/auth/contexts_test.go; the multi-context tests landed ~May 30, so the pollution most likely came from a dev-loop iteration before isolation was in place. - Worse, both the integration and e2e harnesses spawned the real
entirebinary with your live environment: realHOME, noENTIRE_CONFIG_DIR, noXDG_CACHE_HOME, real OS Keychain. - It wasn't just contexts.json:
version_check.json(written viaHOME, ignoringENTIRE_CONFIG_DIRentirely),nodes.json/cluster_cores.json/api_discovery.jsonin~/.cache/entire, andtokens.json/Keychain entries were all exposed.
The fix, two layers:
- In-process safety net (
internal/testdirs): undergo test,contexts.DefaultConfigDir(),discovery.DefaultCacheDir(), versioncheck, and the tokenstore default backend resolve to a throwaway per-process temp dir when their env override is unset — a forgetful test now hits a sandbox, never your home. The tokenstore previously defaulted to your real macOS Keychain in any test package lackingkeyring.MockInit(). - Subprocess isolation:
testing.Testing()is false in spawned binaries, so the integration and e2e TestMains now exportENTIRE_CONFIG_DIR,XDG_CACHE_HOME,ENTIRE_TOKEN_STORE=file+ path, andENTIRE_TEST_AUTH_STORE_FILEprocess-wide; every child (including agent-invoked hooks) inherits them. Versioncheck now honorsENTIRE_CONFIG_DIRso one variable covers the whole config dir.
Rules are documented in CLAUDE.md ("Config/Cache/Keyring Isolation in Tests"), with regression tests asserting test runs never resolve under your real home. mise run fmt, lint (0 issues), and the full test:ci all pass.
One manual cleanup for you: the stale core.example.com entry is still in your real contexts.json — entire auth use core.example.com && entire logout removes it, or just hand-edit ~/.config/entire/contexts.json and delete that entry (the github:toothbrush@localhost:8180 one is presumably your real local-dev login, so I'd leave it).
PR #1411 is up: https://github.com/entireio/cli/pull/1411 — based on main, not stacked.
- This change is independent of the cor-393 stack (#1404/#1410) and was already cut from
origin/main, somainis the right base — a safety fix shouldn't queue behind a draft. - Pushed as
paul/test-config-isolation(the worktree's autogenerated branch name stayed local). - One heads-up for #1410: when the legacy-store demolition lands, the
ENTIRE_TEST_AUTH_STORE_FILEreferences here (e2e/integration TestMains, CLAUDE.md) can be deleted along with it — trivial conflict at worst.
Three audits are running in parallel — one per open PR (#1404, #1410, #1402), each scanning the diff for tests that can reach your real config/cache/keychain without isolation, plus (for #1410) the inevitable overlap with the harness files #1411 just changed. I'll report the consolidated findings when they finish.
#1404 came back clean — its new tests are pure unit tests, and it actually removes env-based auth-URL plumbing from tests. Waiting on the #1410 and #1402 audits.
#1410 also audits green — its new login integration test already passes ENTIRE_CONFIG_DIR + file token store explicitly, and the legacy-store demolition removes surfaces rather than adding them. There's a known, clean merge overlap with #1411 (the authfilestore tag and ENTIRE_TEST_AUTH_STORE_FILE lines become vestigial once the legacy store is deleted). One audit left (#1402).
All three audits are in. Bottom line: no real isolation leaks in any of your open PRs — nothing to fix up. The only action item is merge-order coordination between #1411 and #1410.
Per PR:
-
#1404 (login --server) — clean. New tests are pure unit tests (URL validation, client construction); it actually removes env-based auth-URL usage from tests by passing the server as a parameter.
-
#1410 (demolish auth fallbacks, draft) — clean on isolation. Its new
integration_test/login_test.goalready passesENTIRE_CONFIG_DIR+ENTIRE_TOKEN_STORE=fileto the spawned binary, and deleting the legacy store removes attack surface rather than adding it. The real note: it touches the same two files #1411 just changed — it drops the-tags=authfilestorebuild flag and theENTIRE_TEST_AUTH_STORE_FILEenv var (correctly — the code reading them is deleted). Merge #1411 first, then resolve #1410's conflict by keeping its deletions while preserving #1411's newENTIRE_CONFIG_DIR/XDG_CACHE_HOME/ENTIRE_TOKEN_STORElines. Also worth deleting the "legacy auth store / keyring.MockInit" bullet from CLAUDE.md's new isolation section in that PR, since the legacy store is gone. -
#1402 (repo-scoped tokens) — clean. The auditor flagged
MigrateLegacyLoginContext()calls (repo_token.go:76, data_api.go:85) as potential real-keyring reads, but that's a false positive: those tests live in theauthpackage, whoseTestMainrunskeyring.MockInit()(store_test.go:11-13), so the legacy-store read hits the mock, never your Keychain. Its config/cache access is already env-isolated per test, and after rebasing onto main thetestdirsfallback covers theDefaultCacheDir()call in repo_token.go:80 for any future test that forgets. One heads-up unrelated to isolation: #1402 adds aMigrateLegacyLoginContext()call that #1410 deletes wholesale — whichever lands second has a small semantic conflict to resolve there.
there are some PR comments on https://github.com/entireio/cli/pull/1411. They might be junk, let's consider them in turn.
Verdict on the three Copilot comments: all valid (same root point, not junk), and all addressed in commit d1a2dd033d5 — committed, not pushed.
Taking them in turn:
-
testdirs_test.go:35— "home-prefix assertion is flaky if TMPDIR is under$HOME" — Correct.os.MkdirTemp("", ...)respectsTMPDIR, which some setups point inside the home directory; the test would then fail even though isolation is intact. Fixed: the test now asserts the fallback is never under~/.configor~/.cache— the actual safety property. -
contexts_test.go:308— same issue forDefaultConfigDir— Correct for the same reason. Fixed: now asserts the result is never~/.config/entire(or under it), which is the only off-limits path. -
testdirs.go:11— doc comment overpromises "instead of the real home" — Fair. Reworded the package comment to state the real guarantee ("not~/.config/entireor~/.cache/entire", explicitly notingTMPDIRmay live under$HOME), and aligned the same phrasing in theDefaultConfigDir/DefaultCacheDir/versioncheck doc comments.
One lint fix along the way: revive rejected real as a variable name (shadows the builtin) — renamed to realDir. Affected package tests, fmt, and lint all pass.
The branch is ready to push to update #1411 whenever you want — push yourself or tell me and I'll do it.
okay, push and respond to the review comments - only one sentence each, with link to d1a2dd033d5 as appropriate.
Pushed d1a2dd033d5 to #1411 and replied to all three review comments in-thread, one sentence each with the commit reference.
- testdirs_test.go → "asserts the actual invariant (never under ~/.config or ~/.cache) instead of a $HOME prefix"
- contexts_test.go → "assertion is now 'never ~/.config/entire (or under it)'"
- testdirs.go doc → "reworded: guarantee is 'never the real config/cache locations', TMPDIR may live under $HOME"
PR #1411 is up to date and ready for human review.
Still refused — Secretive needs your physical approval, which only works from your own terminal. Both fixes are committed locally as 7af38daf7f8:
- Renamed
TestDir_StablePerSurfaceAndNeverUnderHome→TestDir_StablePerSurfaceAndNeverRealConfigOrCache - Assertion now compares against
~/.config/entireand~/.cache/entireexactly, not their parents (soTMPDIR=$HOME/.cache/tmpsetups stay green)
To push, run this in the prompt (the ! prefix runs it in this session so you can tap Touch ID):
Once it's up I'll post the one-sentence replies to the two new comments referencing 7af38daf7f8 (waiting so the commit link doesn't 404).
Pushed 7af38daf7f8 and replied to both new comments in-thread with the commit reference. PR #1411 now has all five review comments addressed — the Touch ID approval must have gone through this time.
what's going on in cmd/entire/cli/versioncheck/versioncheck.go globalConfigDirPath? why do we need paths for a version check?
Bottom line: the version check persists a tiny rate-limit/preference cache (version_check.json), and globalConfigDirPath decides where that file lives.
What it's for, concretely (versioncheck.go:40-99):
CheckAndNotifyruns on every CLI invocation (fromroot.go's persistent pre-run). Hitting the GitHub releases API every time would be slow and rude, so it storesLastCheckTimeinversion_check.jsonand skips the network call if the last check was under 24 hours ago.- It also stores
SkippedVersion: if the auto-update prompt offered you a version and you said "skip", that choice has to survive to the next invocation or you'd be nagged about the same release every command. - Both of those need a file that's stable across invocations and independent of any repo — hence the per-user global config dir, not
.entire/in the worktree.
Why I touched it in #1411: it was the odd one out. Everything else resolves the config dir through contexts.DefaultConfigDir() (which honors ENTIRE_CONFIG_DIR), but versioncheck built its own path from os.UserHomeDir() directly — so even a test (or user) that set ENTIRE_CONFIG_DIR still got version_check.json written to the real ~/.config/entire. Since the check runs on every command, every spawned entire in a test harness was a potential write to your real config dir (in practice usually dodged because test binaries report a dev version, which bails early — but that's luck, not isolation). Now it resolves ENTIRE_CONFIG_DIR → testdirs fallback (under go test) → ~/.config/entire, same as everything else.
A fair follow-up question is whether versioncheck should just call contexts.DefaultConfigDir() instead of duplicating the resolution — I kept it separate to avoid coupling cmd/entire/cli/versioncheck to internal/entireclient/contexts for one path join, but consolidating would be a reasonable cleanup if you'd rather have a single resolver.
ok, we should unify that and only implement directory resolution once. however, perhaps it needs a new package - 'contexts' isn't the only concept that needs a config directory.
Full fmt + lint + test:ci is running in the background; I'll commit once it's green. The refactor in summary:
- New
internal/entireclient/userdirs— the single resolver:Config()($ENTIRE_CONFIG_DIR→ testdirs fallback →~/.config/entire) andCache()($XDG_CACHE_HOME/entire→ testdirs fallback →~/.cache/entire), with tests moved there. - Deleted
contexts.DefaultConfigDir()anddiscovery.DefaultCacheDir(); migrated all call sites (auth/contexts.go,context_store.go,control_plane.go,data_api.go,git-remote-entire/main.go,remotehelper/replicas). - versioncheck now delegates to
userdirs.Config()(itsglobalConfigDirPath/cacheFilePathbecame thin, error-free helpers). - tokenstore's file-backend default path (
ENTIRE_TOKEN_STORE=filewith no explicit path) now derives fromuserdirs.Config()too — previously it hard-coded~/.config/entire/tokens.json, ignoringENTIRE_CONFIG_DIR; that's a small behavior fix worth a line in the commit message. - CLAUDE.md now names
userdirsas the only legitimate place to derive these paths.
Done — pushed e5bbf58ddbc and updated the #1411 description to match the branch's full scope.
What happened along the way:
- The remote branch had moved (your
ce433ab3343"fallback" commit makingtestdirs.Dirfail closed viaos.TempDir()instead of returningok=false) — I rebased the unification commit on top of it cleanly. - The lint flapping got a permanent fix: local
mise run lintuses--fix, which strips the//nolint:ireturndirectives that CI's non-fix run needs — so I addedtokenstore.storeto the ireturn allow list in.golangci.yamlinstead of inline directives. - The PR description now covers all four parts: the testdirs safety net (including your fail-closed change), the subprocess env isolation, the
userdirsunification, and the two behavior changes (versioncheck and the token-store default path now honorENTIRE_CONFIG_DIR).
Full fmt/lint/test:ci passed on the pre-rebase tree, and the testdirs package re-passed post-rebase; the rebase only stacked my commit on yours, so CI on the PR will confirm the combination.