Skip to content

test: sandbox mcp query logs - #520

Merged
zzet merged 2 commits into
zzet:mainfrom
aryansk:codex/sandbox-query-log-tests
Aug 9, 2026
Merged

test: sandbox mcp query logs#520
zzet merged 2 commits into
zzet:mainfrom
aryansk:codex/sandbox-query-log-tests

Conversation

@aryansk

@aryansk aryansk commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Put the internal/mcp package's default query-log path under a disposable
    test cache.
  • Preserve and restore existing XDG cache and explicit query-log environment
    values around the package test run.

This prevents concurrent package tests from writing to a developer's real
~/.gortex/cache/query-log.jsonl while keeping individual tests free to
override the logger path with t.Setenv.

Validation

  • gofmt passed.
  • git diff --check passed.
  • go test ./internal/mcp -run 'Test(QueryLogger|CountFromResultText|FirstStringArg)' -count=1 passed.
  • go test ./internal/mcp passed (the first dependency build took about 17
    minutes; the cached test run completed in about 102 seconds).

Closes #518

@zzet

zzet commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Hey @aryansk, thanks for picking this up — and for the careful validation notes in the description. The diagnosis is right, the mechanism is right, and this does close the exact path #518 named: XDG_CACHE_HOME is honoured over the unified layout by platform.unifiedDir, so every CacheDir()-rooted write moves into your temp dir. I measured it — 8 of the 11 files the suite used to create in the real home are gone.

I see, the PR is still not ready for review, however I took a look and I'd like one change before merging, because three files survive.

The package still writes to the real home

~/.gortex/memories/sidecar.sqlite (plus -wal/-shm) is still created on every run of this package.

The reason is that XDG_CACHE_HOME only moves CacheDir(). The surviving writes are DataDir()-rooted, and DataDir() keys off XDG_DATA_HOME (internal/platform/xdg.go:66), which the patch doesn't set:

  • change_contract_riskgate_test.go:38,73 call srv.InitMemories("", "") — the comment says in-memory ack ledger, and for the workspace store that's true;
  • but server.go:1946-1948 also mounts the global store unconditionally: newMemoryManager(platform.MemoriesDir(), "global");
  • MemoriesDir() is <DataDir>/memories, i.e. the developer's real ~/.gortex/memories.

Repro that doesn't depend on what's already in your home — point HOME somewhere observable and see what the suite builds there:

go test -c -o /tmp/mcp.test ./internal/mcp/
FAKE=$(mktemp -d)
( cd internal/mcp && env -u XDG_CACHE_HOME -u XDG_DATA_HOME -u XDG_CONFIG_HOME \
    -u GORTEX_QUERY_LOG HOME="$FAKE" /tmp/mcp.test -test.count=1 >/dev/null 2>&1 )
find "$FAKE" -type f

On main that prints 11 gortex files; on this branch it prints 3, all under .gortex/memories/. Skipping TestRiskGateAckLifecycle|TestRiskGateOffByDefault prints none.

This matters more than "one leftover path", because cmd/gortex's guard can't see it. realUserStatePaths() (cmd/gortex/main_test.go:60-72) is a whitelist of 8 paths and covers nothing under DataDir. So as it stands this PR would turn #518 green while the invariant the guard exists to protect is still violated — which is the failure mode we'd least like to ship, since the next person has no signal at all.

Suggested change

internal/testenv.SandboxProcess() is the TestMain-time counterpart of the per-test helpers, added for exactly this shape and already used by cmd/gortex/main_test.go:35. It moves HOME/USERPROFILE, the AppData pair, all five XDG_* variables and the GORTEX_DAEMON_* paths, and it handles the save/restore and cleanup you're doing by hand. internal/testenv has no internal dependencies, so there's no import cycle from internal/mcp.

func TestMain(m *testing.M) {
	os.Setenv(profiles.ActiveEnv, profiles.DefaultName)

	// NewServer builds the query logger and mounts the global memory store
	// eagerly, so the package needs a sandboxed home — redirecting the cache
	// alone leaves <DataDir>/memories on the developer's real ~/.gortex.
	restore, err := testenv.SandboxProcess()
	if err != nil {
		fmt.Fprintf(os.Stderr, "internal/mcp: cannot sandbox the test environment: %v\n", err)
		os.Exit(1)
	}
	// query_log.go lets an ambient GORTEX_QUERY_LOG beat CacheDir, so clear it
	// and let the logger resolve the sandboxed cache directory.
	os.Unsetenv("GORTEX_QUERY_LOG")

	code := m.Run()
	restore()
	os.Exit(code)
}

Imports become fmt, os, testing, require, profiles, internal/testenvpath/filepath is no longer needed. I ran the full package this way and it stays green, with nothing created under a fake HOME.

Worth keeping the explicit GORTEX_QUERY_LOG handling you added, by the way — you're right that it's load-bearing. SandboxProcess doesn't touch that variable, and query_log.go:124 lets it win over CacheDir(), so a developer who exports it would still write through. Unsetting it inside the sandbox is the smaller version of the same instinct.

Not this PR

Flagging these so the scope stays where you put it — please don't feel obliged to pick them up, I'll file them separately:

  • realUserStatePaths() should learn about DataDir-rooted state, or "the guard is green" keeps meaning less than it looks.
  • internal/serverstack has no TestMain at all and builds the same server — it's the production caller of InitMemories (shared_server.go:646), so it reproduces this class too.
  • internal/embedding writes ~87 MB into the real ~/.gortex/models; the -race skip at provider_test.go:134 keeps CI from ever executing it.

One nit

os.Exit(1) on the MkdirTemp failure path leaves no diagnostic, so go test ./internal/mcp/ would report FAIL with nothing to explain it. The snippet above prints the error first.

Also note CI hasn't run on this branch yet (no checks reported — fork PRs need a maintainer to approve the workflow), so the description's validation is the only signal we have so far. I'll get the workflows approved so the matrix has a look before this goes in.

@aryansk

aryansk commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. I updated the package TestMain to use the existing internal/testenv.SandboxProcess(), which redirects HOME/USERPROFILE and all XDG roots so the DataDir-backed global memory store is sandboxed too. I also unset GORTEX_QUERY_LOG so an ambient override cannot bypass the sandbox. The full suite now passes with go test ./internal/mcp/ -count=1 (141.885s). The follow-up is pushed at a9522775c1821e9cfb715a8e0ea0af02920049e8.

@zzet
zzet marked this pull request as ready for review August 9, 2026 17:02
@zzet
zzet merged commit d21a449 into zzet:main Aug 9, 2026
11 checks passed
@zzet

zzet commented Aug 9, 2026

Copy link
Copy Markdown
Owner

@aryansk thank you for your contribution!

@aryansk

aryansk commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the kind note and for the careful review. I appreciate the guidance on sandboxing both the cache- and data-backed paths, and I’m glad the final test isolation now covers the full surface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

internal/mcp tests write the real ~/.gortex/cache/query-log.jsonl, tripping cmd/gortex's user-state guard in multi-package runs

2 participants