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.
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
+1
-2
@@ -145,8 +145,7 @@
|
||||
"claude_desktop": ">=1.0.0",
|
||||
"platforms": [
|
||||
"darwin",
|
||||
"linux",
|
||||
"win32"
|
||||
"linux"
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user