From 61d46b54ee45f77ffde7cb9540048f0acba70abe Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sun, 17 May 2026 21:18:05 -0700 Subject: [PATCH] fix(mcp): dedup PYTHONPATH and drop unsupported win32 platform Addresses two Greptile findings on #428. P1 - buildEnv duplicated PYTHONPATH when the parent environment already set one. POSIX getenv returns the first match, so the user's stale PYTHONPATH would shadow the engine's cache dir and break `from lib import ...` with ModuleNotFoundError. buildEnv now filters any incoming PYTHONPATH= entry before appending the cache dir. Adds TestRunDropsPreExistingPythonPath (end-to-end through the stub interpreter) and TestBuildEnvDropsAllPreExistingPythonPath (direct unit on the helper) to cover the missed case. P2 - manifest.compatibility.platforms listed "win32" even though the release matrix doesn't ship a Windows binary; Claude Desktop would let Windows users start an install with no matching artifact. Removed until the Windows packaging follow-up lands. Manifest test renamed to TestPlatformsMatchShippingMatrix and tightened: now forbids platforms the release CI doesn't build, with a message pointing at .github/workflows/release.yml. go test ./... 38 passed across 4 packages. --- mcp/internal/engine/run.go | 22 +++++++--- mcp/internal/engine/run_test.go | 58 ++++++++++++++++++++++++++ mcp/internal/manifest/manifest_test.go | 20 ++++++--- mcp/manifest.json | 3 +- 4 files changed, 91 insertions(+), 12 deletions(-) diff --git a/mcp/internal/engine/run.go b/mcp/internal/engine/run.go index 7c84fd0..4933966 100644 --- a/mcp/internal/engine/run.go +++ b/mcp/internal/engine/run.go @@ -9,6 +9,7 @@ import ( "os/exec" "path/filepath" "runtime" + "strings" "time" ) @@ -140,12 +141,23 @@ func resolveTimeout(explicit time.Duration) time.Duration { return DefaultTimeout } -// buildEnv stitches PYTHONPATH onto os.Environ + ExtraEnv. The engine's -// `from lib import ...` statements resolve because lib/ sits next to -// last30days.py inside CacheDir. +// buildEnv stitches PYTHONPATH onto os.Environ + ExtraEnv. Any pre-existing +// PYTHONPATH in the parent environment is dropped before appending the +// cache dir; otherwise the child sees two PYTHONPATH= entries and POSIX +// getenv returns the first one, so the user's value wins and the engine's +// `from lib import ...` fails with ModuleNotFoundError. The engine is +// self-contained and does not need the user's Python module search path. func buildEnv(cacheDir string, extra []string) []string { - base := os.Environ() - base = append(base, "PYTHONPATH="+cacheDir) + const pyKey = "PYTHONPATH=" + parent := os.Environ() + base := make([]string, 0, len(parent)+1+len(extra)) + for _, kv := range parent { + if strings.HasPrefix(kv, pyKey) { + continue + } + base = append(base, kv) + } + base = append(base, pyKey+cacheDir) base = append(base, extra...) return base } diff --git a/mcp/internal/engine/run_test.go b/mcp/internal/engine/run_test.go index a8393c5..1e71f59 100644 --- a/mcp/internal/engine/run_test.go +++ b/mcp/internal/engine/run_test.go @@ -117,6 +117,64 @@ func TestRunSetsPythonPath(t *testing.T) { } } +// TestRunDropsPreExistingPythonPath guards the buildEnv dedup: when the +// parent already sets PYTHONPATH (common on dev machines and CI runners +// that touch Python), the child must NOT see two PYTHONPATH= entries. +// POSIX getenv returns the first match, so a duplicate from os.Environ +// would shadow our cache-dir entry and break `from lib import ...`. +func TestRunDropsPreExistingPythonPath(t *testing.T) { + stub := makeStubPython(t) + cache := stageCache(t) + t.Setenv("PYTHONPATH", "/users-stale-pythonpath") + t.Setenv("STUB_ECHO_ENV", "PYTHONPATH") + + res, err := Run(context.Background(), RunOptions{ + PythonPath: stub, + CacheDir: cache, + }) + if err != nil { + t.Fatalf("Run: %v", err) + } + got := strings.TrimSpace(string(res.Stdout)) + want := "PYTHONPATH=" + cache + if got != want { + t.Fatalf("stdout = %q, want %q (stale parent value leaked through)", got, want) + } +} + +func TestBuildEnvDropsAllPreExistingPythonPath(t *testing.T) { + // Direct unit test on buildEnv to catch the case where the parent has + // PYTHONPATH set: the returned slice must contain exactly one + // PYTHONPATH= entry, and it must be ours. + t.Setenv("PYTHONPATH", "/parent/one") + cache := "/cache/dir" + out := buildEnv(cache, []string{"EXTRA=1"}) + + var pythonPaths []string + for _, kv := range out { + if strings.HasPrefix(kv, "PYTHONPATH=") { + pythonPaths = append(pythonPaths, kv) + } + } + if len(pythonPaths) != 1 { + t.Fatalf("got %d PYTHONPATH entries, want 1: %v", len(pythonPaths), pythonPaths) + } + if pythonPaths[0] != "PYTHONPATH="+cache { + t.Fatalf("PYTHONPATH = %q, want %q", pythonPaths[0], "PYTHONPATH="+cache) + } + // Confirm ExtraEnv still rides along. + found := false + for _, kv := range out { + if kv == "EXTRA=1" { + found = true + break + } + } + if !found { + t.Fatal("EXTRA=1 missing from buildEnv output") + } +} + func TestRunSurfacesExitCode(t *testing.T) { stub := makeStubPython(t) cache := stageCache(t) diff --git a/mcp/internal/manifest/manifest_test.go b/mcp/internal/manifest/manifest_test.go index 69a3bb7..26a2c98 100644 --- a/mcp/internal/manifest/manifest_test.go +++ b/mcp/internal/manifest/manifest_test.go @@ -154,15 +154,25 @@ func TestUserConfigShape(t *testing.T) { } } -func TestPlatformsCoverDesktopTargets(t *testing.T) { +func TestPlatformsMatchShippingMatrix(t *testing.T) { + // compatibility.platforms must list exactly what the release CI + // actually packages. Listing a platform we don't ship would let + // Claude Desktop start an install that has no matching binary inside + // the bundle, producing a silent failure. The CI matrix in + // .github/workflows/release.yml currently covers darwin (arm64 + + // amd64) and linux/amd64; Windows is deferred. m := loadManifest(t) - want := map[string]bool{"darwin": false, "linux": false, "win32": false} + required := map[string]bool{"darwin": false, "linux": false} + forbidden := map[string]bool{"win32": true} for _, p := range m.Compatibility.Platforms { - if _, expected := want[p]; expected { - want[p] = true + if _, ok := required[p]; ok { + required[p] = true + } + if forbidden[p] { + t.Errorf("compatibility.platforms contains %q but the release matrix does not ship that platform; add it to the matrix or remove from the manifest", p) } } - for p, found := range want { + for p, found := range required { if !found { t.Errorf("compatibility.platforms missing %q", p) } diff --git a/mcp/manifest.json b/mcp/manifest.json index 63b8065..71f2afa 100644 --- a/mcp/manifest.json +++ b/mcp/manifest.json @@ -145,8 +145,7 @@ "claude_desktop": ">=1.0.0", "platforms": [ "darwin", - "linux", - "win32" + "linux" ] } }