test(mcp): Isolate MCP test config - #176
Conversation
Prevent registry, cache, and OAuth state from leaking into the real user configuration directory on macOS, eliminating stale cross-test state. Claude-Session-Id: 15389a35-778b-486b-bb10-0be18fee5b76
Gavel resultsGavel exited with code 1. |
WalkthroughMCP tests now use a shared helper that isolates ChangesMCP test configuration isolation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/client_e2e_test.go`:
- Line 16: Update isolateConfigHome to avoid changing HOME when both GOPATH and
GOMODCACHE are unset, preserving the child go run’s existing module cache
behavior. Instead, preserve one of those environment variables or isolate only
the application configuration while leaving HOME unchanged.
In `@mcp/client_registry_test.go`:
- Around line 13-23: Update isolateConfigHome to set the Windows environment
variables AppData and USERPROFILE to directories under the test’s temporary
home, alongside HOME and XDG_CONFIG_HOME. Ensure os.UserConfigDir and
os.UserHomeDir resolve entirely within the isolated test directory on Windows.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ec12495-88c5-4034-9579-7a5c1bbe45a3
📒 Files selected for processing (5)
mcp/client_command_test.gomcp/client_e2e_test.gomcp/client_oauth_test.gomcp/client_registry_test.gomcp/client_run_test.go
|
|
||
| func TestClientCommandsStdioAndOfflineHelp(t *testing.T) { | ||
| t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | ||
| isolateConfigHome(t) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'go run|executeMCPCommand|exec\.Cmd|GOPATH|GOMODCACHE|Env:' \
mcp --glob '*.go' --glob 'go.mod'Repository: flanksource/clicky
Length of output: 14017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- isolateConfigHome definition and uses ---'
rg -n -C 12 'func isolateConfigHome|isolateConfigHome\(' . --glob '*.go'
printf '%s\n' '--- subprocess construction and environment handling ---'
rg -n -C 14 'exec\.Command(Context)?|Cmd\.Env|os\.Environ|GOPATH|GOMODCACHE|HOME|ServerConfig|Command:' mcp --glob '*.go'
printf '%s\n' '--- module and workspace metadata ---'
find . -maxdepth 2 \( -name go.mod -o -name go.work \) -print -exec sed -n '1,100p' {} \;Repository: flanksource/clicky
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- stdio transport implementation ---'
sed -n '83,135p' mcp/client_dial.go
printf '%s\n' '--- test server module metadata ---'
find mcp/testdata -maxdepth 3 -type f -print -exec sh -c 'echo "--- $1"; sed -n "1,120p" "$1"' sh {} \;
printf '%s\n' '--- dependency versions and Go directives ---'
sed -n '1,140p' go.mod
rg -n 'New.*Stdio|Stdio|Command.*Args|cfg\.Env|exec\.' . --glob '*.go' --glob '!vendor/**'Repository: flanksource/clicky
Length of output: 19514
🌐 Web query:
github mark3labs mcp-go v0.56.0 transport.NewStdio environment os.Environ
💡 Result:
In the mark3labs/mcp-go library, specifically as of version 0.56.0, the transport.NewStdio function (and related helpers) handles environment variables by merging provided custom environment variables with the host process's existing environment variables [1][2]. When spawning a subprocess, the transport implementation uses the following logic to set the environment [1]: cmd.Env = append(os.Environ, c.env...) This means: 1. It retrieves the host's current environment variables using os.Environ [1]. 2. It appends the slice of custom environment variables (provided via the env argument in NewStdio or NewStdioWithOptions) to the host environment [1]. 3. The resulting environment is then passed to the exec.Cmd instance used to launch the subprocess [1]. If you need finer control over the process creation—such as replacing the environment entirely rather than appending to it, or configuring other low-level system attributes—the library provides the NewStdioWithOptions function [1][3]. By using this with the WithCommandFunc option, you can provide a custom factory function to define exactly how the exec.Cmd is constructed and configured, including full control over its Env field [4][3].
Citations:
- 1: https://github.com/mark3labs/mcp-go/blob/b2fb8ba1/client/transport/stdio.go
- 2: mark3labs/mcp-go@v0.50.0...v0.56.0
- 3: Support creating an
Stdioclient with options mark3labs/mcp-go#457 - 4: https://mcp-go.dev/transports/stdio/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
url='https://raw.githubusercontent.com/mark3labs/mcp-go/v0.56.0/client/transport/stdio.go'
if command -v curl >/dev/null 2>&1; then
curl -fsSL "$url" | sed -n '1,240p'
else
echo 'curl unavailable' >&2
exit 1
fiRepository: flanksource/clicky
Length of output: 6996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os
import subprocess
import tempfile
def go_env(home, extra):
env = os.environ.copy()
env["HOME"] = home
env.pop("GOPATH", None)
env.pop("GOMODCACHE", None)
env.update(extra)
out = subprocess.check_output(
["go", "env", "GOPATH", "GOMODCACHE"],
env=env,
text=True,
).splitlines()
return dict(zip(("GOPATH", "GOMODCACHE"), out))
with tempfile.TemporaryDirectory() as home:
print("unset GOPATH/GOMODCACHE:", go_env(home, {}))
print("fixed GOPATH:", go_env(home, {"GOPATH": "/stable/gopath"}))
print("fixed GOMODCACHE:", go_env(home, {"GOMODCACHE": "/stable/modcache"}))
print("parent GOPATH:", os.environ.get("GOPATH", "<unset>"))
print("parent GOMODCACHE:", os.environ.get("GOMODCACHE", "<unset>"))
PYRepository: flanksource/clicky
Length of output: 507
Preserve GOPATH or GOMODCACHE
When both variables are unset, changing HOME makes the child go run use a temporary module cache. Preserve one variable or isolate application configuration without changing HOME.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/client_e2e_test.go` at line 16, Update isolateConfigHome to avoid
changing HOME when both GOPATH and GOMODCACHE are unset, preserving the child go
run’s existing module cache behavior. Instead, preserve one of those environment
variables or isolate only the application configuration while leaving HOME
unchanged.
| // isolateConfigHome points NewServerRegistry at a per-test directory on every | ||
| // platform. os.UserConfigDir only reads XDG_CONFIG_HOME on unix-like systems; | ||
| // on darwin it derives ~/Library/Application Support from HOME, so setting only | ||
| // XDG_CONFIG_HOME leaks registry, cache, and OAuth state into the real user | ||
| // config directory and makes later runs fail on stale state. | ||
| func isolateConfigHome(t *testing.T) { | ||
| t.Helper() | ||
| home := t.TempDir() | ||
| t.Setenv("HOME", home) | ||
| t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'User(Config|Cache|Home)Dir|XDG_(CONFIG|CACHE)_HOME|AppData|LocalAppData|USERPROFILE|GOOS|windows' \
--glob '*.go' --glob 'go.mod' --glob '*.yml' --glob '*.yaml' --glob 'Dockerfile*' .Repository: flanksource/clicky
Length of output: 16977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test helper and registry call sites ---'
cat -n mcp/client_registry_test.go | sed -n '1,220p'
printf '%s\n' '--- registry path implementation ---'
cat -n mcp/client_registry.go | sed -n '235,275p'
printf '%s\n' '--- related config/cache/home helpers ---'
rg -n -C 8 'GetConfigPathFor|GetPromptsPathFor|UserConfigDir|UserCacheDir|UserHomeDir|\\.cache|mcp-config|mcp-prompts|OAuth|oauth' mcp ai --glob '*.go'
printf '%s\n' '--- Go stdlib source locations and relevant implementations ---'
if command -v go >/dev/null 2>&1; then
goroot="$(go env GOROOT)"
printf 'GOROOT=%s\n' "$goroot"
rg -n -C 12 'func User(Config|Cache|Home)Dir|XDG_CONFIG_HOME|XDG_CACHE_HOME|LocalAppData|APPDATA|USERPROFILE|Library/Application Support' \
"$goroot/src/os" "$goroot/src/internal" 2>/dev/null || true
else
printf '%s\n' 'go command unavailable'
fiRepository: flanksource/clicky
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if ! command -v go >/dev/null 2>&1; then
echo "go command unavailable"
exit 0
fi
goroot="$(go env GOROOT)"
printf '%s\n' '--- UserConfigDir/UserCacheDir/UserHomeDir implementations ---'
rg -l 'func User(Config|Cache|Home)Dir' "$goroot/src/os" | while IFS= read -r f; do
echo "FILE: $f"
rg -n -A45 -B5 'func User(Config|Cache|Home)Dir' "$f"
done
printf '%s\n' '--- helper call sites ---'
rg -n -C 3 'isolateConfigHome\(' mcp --glob '*.go'
printf '%s\n' '--- all NewServerRegistry uses in tests ---'
rg -n -C 3 'NewServerRegistry\(' --glob '*_test.go' .Repository: flanksource/clicky
Length of output: 11228
Redirect Windows user directories in isolateConfigHome.
On Windows, os.UserConfigDir() reads %AppData% and os.UserHomeDir() reads %USERPROFILE%. Set AppData and USERPROFILE to paths under home; otherwise tests can write registry, cache, or OAuth state to the real user directory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/client_registry_test.go` around lines 13 - 23, Update isolateConfigHome
to set the Windows environment variables AppData and USERPROFILE to directories
under the test’s temporary home, alongside HOME and XDG_CONFIG_HOME. Ensure
os.UserConfigDir and os.UserHomeDir resolve entirely within the isolated test
directory on Windows.
1 participant
What
Why
Summary by CodeRabbit