Fix Test Fixture Leaking Into User Config

Claude Code·Fable 5.[1m]·toothbrush·3mo ago·4hr 9min·4 Checkpoints·34 file changes·+398/-169·326.4K tokens

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

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.

3mo ago·20m

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) — under go 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 honors ENTIRE_CONFIG_DIR — it previously only used HOME), 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 spawned entire binaries can't detect they're under test. This was the confirmed gap: both harnesses passed your real environment through.
3mo ago·1m

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. The core.example.com/bob fixture matches cmd/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 entire binary with your live environment: real HOME, no ENTIRE_CONFIG_DIR, no XDG_CACHE_HOME, real OS Keychain.
  • It wasn't just contexts.json: version_check.json (written via HOME, ignoring ENTIRE_CONFIG_DIR entirely), nodes.json/cluster_cores.json/api_discovery.json in ~/.cache/entire, and tokens.json/Keychain entries were all exposed.

The fix, two layers:

  • In-process safety net (internal/testdirs): under go 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 lacking keyring.MockInit().
  • Subprocess isolation: testing.Testing() is false in spawned binaries, so the integration and e2e TestMains now export ENTIRE_CONFIG_DIR, XDG_CACHE_HOME, ENTIRE_TOKEN_STORE=file + path, and ENTIRE_TEST_AUTH_STORE_FILE process-wide; every child (including agent-invoked hooks) inherits them. Versioncheck now honors ENTIRE_CONFIG_DIR so 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).

let's push this and create a stacked PR (on which branch would we put it?)

3mo ago·2m

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, so main is 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_FILE references here (e2e/integration TestMains, CLAUDE.md) can be deleted along with it — trivial conflict at worst.

in my open PRs, would we need to fix up more spots where test isolation is leaking?

3mo ago·1m

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.

3mo ago·8.1s

#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.

3mo ago·14.0s

#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).

3mo ago·39.3s

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.go already passes ENTIRE_CONFIG_DIR + ENTIRE_TOKEN_STORE=file to 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=authfilestore build flag and the ENTIRE_TEST_AUTH_STORE_FILE env var (correctly — the code reading them is deleted). Merge #1411 first, then resolve #1410's conflict by keeping its deletions while preserving #1411's new ENTIRE_CONFIG_DIR/XDG_CACHE_HOME/ENTIRE_TOKEN_STORE lines. 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 the auth package, whose TestMain runs keyring.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 the testdirs fallback covers the DefaultCacheDir() call in repo_token.go:80 for any future test that forgets. One heads-up unrelated to isolation: #1402 adds a MigrateLegacyLoginContext() 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.

3mo ago·3m

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:

  1. testdirs_test.go:35 — "home-prefix assertion is flaky if TMPDIR is under $HOME" — Correct. os.MkdirTemp("", ...) respects TMPDIR, 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 ~/.config or ~/.cache — the actual safety property.

  2. contexts_test.go:308 — same issue for DefaultConfigDir — Correct for the same reason. Fixed: now asserts the result is never ~/.config/entire (or under it), which is the only off-limits path.

  3. testdirs.go:11 — doc comment overpromises "instead of the real home" — Fair. Reworded the package comment to state the real guarantee ("not ~/.config/entire or ~/.cache/entire", explicitly noting TMPDIR may live under $HOME), and aligned the same phrasing in the DefaultConfigDir/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.

3mo ago·55.5s

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.

3mo ago
3mo ago

okay, there are more comments, and i actually agree with them.

3mo ago·2m

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/entire and ~/.cache/entire exactly, not their parents (so TMPDIR=$HOME/.cache/tmp setups 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).

push again

3mo ago·1m

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?

3mo ago·26.5s

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):

  • CheckAndNotify runs on every CLI invocation (from root.go's persistent pre-run). Hitting the GitHub releases API every time would be slow and rude, so it stores LastCheckTime in version_check.json and 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.

3mo ago·8m

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) and Cache() ($XDG_CACHE_HOME/entire → testdirs fallback → ~/.cache/entire), with tests moved there.
  • Deleted contexts.DefaultConfigDir() and discovery.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() (its globalConfigDirPath/cacheFilePath became thin, error-free helpers).
  • tokenstore's file-backend default path (ENTIRE_TOKEN_STORE=file with no explicit path) now derives from userdirs.Config() too — previously it hard-coded ~/.config/entire/tokens.json, ignoring ENTIRE_CONFIG_DIR; that's a small behavior fix worth a line in the commit message.
  • CLAUDE.md now names userdirs as the only legitimate place to derive these paths.
3mo ago·4m

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 making testdirs.Dir fail closed via os.TempDir() instead of returning ok=false) — I rebased the unification commit on top of it cleanly.
  • The lint flapping got a permanent fix: local mise run lint uses --fix, which strips the //nolint:ireturn directives that CI's non-fix run needs — so I added tokenstore.store to the ireturn allow list in .golangci.yaml instead 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 userdirs unification, and the two behavior changes (versioncheck and the token-store default path now honor ENTIRE_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.