test: isolate tests from ambient XDG and GOG_* path variables - #997
Conversation
Tests isolate storage via per-test HOME/t.TempDir sandboxes, but the
layout resolver (internal/config/layout.go) honors GOG_HOME, the
GOG_{CONFIG,DATA,STATE,CACHE}_DIR overrides, and the XDG base directory
variables ahead of HOME-derived defaults. On machines that export any
of them, tests resolve the developer's real gogcli directories: with
XDG_DATA_HOME/XDG_STATE_HOME exported, 19 failures across internal/cmd,
internal/config, and internal/secrets from cross-test contamination
(the exact split depends on preexisting state and platform); with
GOG_HOME exported, 77+ failures. In every case test fixtures
(service-account stubs, tracking.json, gmail-watch state, file-keyring
entries) leak into the real directories, clobbering any real
file-keyring, tracking, or watch state. CI never sees this because
GitHub runners export none of these variables.
Unset the GOG_* path overrides plus XDG data/state/cache in
internal/cmd's TestMain (which already redirects HOME and
XDG_CONFIG_HOME to a temp root), add equivalent TestMains to
internal/secrets and internal/config, and clear the GOG_* overrides in
the internal/googleapi test that deliberately writes to the ambient
layout (with GOG_HOME exported it previously stayed green while
writing into the real directory). Unsetting rather than redirecting
matters: a single shared override directory still cross-contaminates
tests; unsetting lets each test's own sandbox take effect, matching CI
behavior.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed August 21, 2026, 11:11 PM ET / August 22, 2026, 03:11 UTC. ClawSweeper reviewWhat this changesThe PR makes command, config, secrets, and service-account tests ignore ambient gogcli/XDG storage-path overrides so fixtures remain in temporary test sandboxes. Regression provenancePossible regression — probable (reviewed change; reproduction). No predecessor PR is attributed. Merge readinessKeep open for normal maintainer merge review: current main still lets ambient storage overrides bypass test sandboxes, while this test-only patch cleanly closes that gap and has sufficient terminal proof. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togethergogcli’s layout resolver reads environment-based storage locations and supplies config, data, state, and cache paths to commands and credential stores. These test packages use that resolver while creating fixtures, so their setup must prevent inherited paths from directing test writes into developer storage. flowchart LR
A[Ambient path variables] --> B[Package test setup]
B --> C[Layout resolver]
C --> D[Test storage paths]
D --> E[Fixture and state writes]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Merge the focused test setup changes so ambient, documented storage overrides cannot direct unit-test fixtures into developer gogcli state. Do we have a high-confidence way to reproduce the issue? Yes—current source proves that documented ambient paths outrank the existing test sandboxes, and the PR provides a no-credential terminal matrix for that path; this review did not execute it because the checkout must remain read-only. Is this the best way to solve the issue? Yes—the package-level cleanup preserves intentional per-test overrides while preventing inherited process settings from selecting shared storage, without changing runtime resolution behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ab7e0ed706f9. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
1 participant
What
Makes the test suite immune to ambient path-environment variables. The layout resolver (
internal/config/layout.go) honorsGOG_HOME,GOG_{CONFIG,DATA,STATE,CACHE}_DIR, and the XDG base directories ahead of HOME-derived defaults, but tests isolate themselves with per-testt.Setenv("HOME", t.TempDir())sandboxes. On any machine that exports one of these documented variables,go test ./...today both fails (cross-test contamination through the shared real directory) and writes test fixtures into the developer's real gogcli data — including live file-keyring entries,tracking.json, and gmail-watch state.Four test-only changes, no runtime code touched:
internal/cmd/testmain_test.go— the existingTestMain(which already redirectsHOMEandXDG_CONFIG_HOMEto a temp root, from 2ca93e9) now also unsets the fiveGOG_*path overrides plusXDG_DATA_HOME/XDG_STATE_HOME/XDG_CACHE_HOME, restoring saved values afterward.internal/config/testmain_test.go,internal/secrets/testmain_test.go(new) — minimalTestMains unsetting all nine path variables; these packages' tests were exposed the same way.internal/googleapi/service_account_test.go—TestTokenSourceForServiceAccountScopesUsesInjectedStoredeliberately writes an "ambient" fixture through the real resolver to prove the injected store wins. It already pinsHOME/XDG_CONFIG_HOME/XDG_DATA_HOMEper test but notGOG_*, so withGOG_HOMEexported it wrote<GOG_HOME>/data/sa-YUBiLmNvbQ.json(contents:ambient) into the real directory while reportingok— silently clobbering any real stored service-account key for that address. It now clears theGOG_*overrides too.Unsetting rather than redirecting is deliberate: we tried redirecting the variables at a single shared package-level directory, and tests still cross-contaminate through it — the failures need no preexisting content, because writer tests fill the shared directory mid-run and reader tests then see their state (preexisting junk only changes which package the failures land in). That is also the precise reason CI has never seen this: GitHub runners export none of these variables, so every test falls back to its own
t.Setenv("HOME", …)sandbox — had a runner exportedXDG_DATA_HOME, even a pristine one, the same failures would appear. Unsetting reproduces that environment everywhere. Per-testt.Setenvof any of these variables keeps working (TestMainruns beforem.Run), and the build-tagged integration suites that intentionally target the real layout are untouched.Why
Measured at current main (45b5d76), on macOS (the resolver branches involved are not platform-gated, so Linux with the same variables exported is equally exposed):
XDG_DATA_HOME/XDG_STATE_HOMEexported → 19 failing tests acrossinternal/cmd,internal/config,internal/secrets(the split varies with what's already in the shared directory), plus service-account stubs, a file keyring,tracking.json, and gmail-watch state written into the real$XDG_DATA_HOME/gogcliand$XDG_STATE_HOME/gogcli.GOG_HOMEexported → 77 failing tests, same mechanism, higher resolver precedence — andGOG_HOMEis gogcli's own documented relocation knob, so the population most at risk is gogcli developers who also use gogcli.internal/googleapileak above stays green while overwriting real data.This came out of a real diagnosis: on a Nix-managed dev machine (XDG variables exported globally), 19 tests failed on a clean checkout of main, and the real
~/.local/share/gogcli/~/.local/state/gogclihad been silently accumulating test fixtures since June. VISION.md counts reliability improvements around keyring and credentials as wanted work; this protects contributors' actual credentials/state fromgo test.Behavior changes (complete ledger)
_test.gofiles.GOG_HOME,GOG_{CONFIG,DATA,STATE,CACHE}_DIR,XDG_DATA_HOME,XDG_STATE_HOME,XDG_CACHE_HOME(and, ininternal/config/internal/secrets,XDG_CONFIG_HOME). Tests that set these per test are unaffected.XDG_CACHE_HOMEunset, thego buildsubprocess ininternal/cmd's slides-assets test derives its build cache under the sandboxed HOME on Linux (cold cache per run). No measurable runtime change on darwin; the unset is still wanted because gogcli genuinely resolves the cache path (internal/cmd/backup_gmail.go).Proof
Self-contained TAP script, no credentials required — it runs the matrix against whatever checkout it's started from, so the same script demonstrates the bug on main and its absence here. It pins
GOFLAGSand starts each scenario from all nine path variables unset (setting only that scenario's), so ambient environment on the machine running it cannot skew or vacuously pass the checks.proof-isolation.sh(bash, stdlib only)At current main (45b5d76):
The silent case in isolation, at main — the package reports
okwhile writing through the real resolver (sa-YUBiLmNvbQ.jsonis the service-account stub fora@b.com, base64url-encoded):On this branch:
Scope notes: the proof exercises the four affected packages; a full
go test ./...under each of the three environments also passes on this branch (that is how the affected set was established — no other package resolves the system layout outside build-tagged integration tests, which intentionally use the real one). Windows CI runs with none of these variables set, so it sees pure CI-parity behavior. One adjacent observation, deliberately out of scope for this PR: CI cannot detect removal of theseTestMains (runners never export the variables), so the isolation is convention-guarded only. (The keyring-selection variables —GOG_KEYRING_BACKENDand friends — were audited separately and need no scrubbing here: every test that opens a secrets store already pins the backend tofileper test, on main and on this branch alike.)🤖 Generated with Claude Code