fix(scanner): npx resolver prefers real package source over tools.json stub (#661)

The npx cache can hold the same package under multiple hashes: one with the
real installed source (package.json + dist/*.js) and another with only a
tools.json stub that mcpproxy writes when dumping tool definitions. The stub's
mtime is usually newer, so resolveNpxCache's newest-mtime heuristic picked it
and the scan reported false coverage (1 stub file instead of ~30 real files).

Rank candidates by real-source-ness first (package.json, a dist/lib/build/src
subdir, or a JS/TS file at the root), falling back to the newest-mtime tiebreak
only within the same class. When EVERY candidate is a bare stub, return an error
instead of the stub so the published-source fetch fallback added in MCP-2206
(#658) fetches the real source — rather than short-circuiting it with false
1-file coverage.

Related MCP-2397

Co-authored-by: Paperclip <noreply@paperclip.ing>

Author: Dumbris

internal/security/scanner/source_resolver.go

@@ -1177,8 +1177,18 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro
 	}
 
 	// Search for the package in npx cache: ~/.npm/_npx/<hash>/node_modules/<package>/
+	//
+	// The SAME package name can appear under multiple npx cache hashes. One may
+	// hold the real installed source (package.json + dist/lib/*.js) while another
+	// holds only a tools.json stub that mcpproxy itself wrote into the cache when
+	// it dumped tool definitions. The stub's mtime is frequently NEWER than the
+	// real source (it was just written), so a naive newest-mtime pick selects the
+	// stub and the scan reports a false "1 file" coverage (MCP-2397). We therefore
+	// prefer candidates that look like real package source, and only fall back to
+	// the newest-mtime tiebreak among candidates of the same class.
 	var bestMatch string
 	var bestModTime int64
+	var bestReal bool
 
 	entries, err := os.ReadDir(npxCacheDir)
 	if err != nil {
@@ -1191,22 +1201,51 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro
 		}
 		candidatePath := filepath.Join(npxCacheDir, entry.Name(), "node_modules", pkgName)
 		stat, err := os.Stat(candidatePath)
-		if err != nil {
+		if err != nil || !stat.IsDir() {
 			continue
 		}
-		if stat.IsDir() {
-			modTime := stat.ModTime().Unix()
-			if modTime > bestModTime {
-				bestModTime = modTime
-				bestMatch = candidatePath
-			}
+		real := npxCacheLooksReal(candidatePath)
+		modTime := stat.ModTime().Unix()
+
+		// Selection order:
+		//  1. any real-source candidate beats any stub;
+		//  2. within the same class, the newest mtime wins.
+		better := false
+		switch {
+		case bestMatch == "":
+			better = true
+		case real && !bestReal:
+			better = true
+		case real == bestReal && modTime > bestModTime:
+			better = true
+		}
+		if better {
+			bestMatch = candidatePath
+			bestModTime = modTime
+			bestReal = real
 		}
 	}
 
 	if bestMatch == "" {
 		return nil, fmt.Errorf("package %q not found in npx cache (%s)", pkgName, npxCacheDir)
 	}
 
+	if !bestReal {
+		// Every candidate was a bare stub (e.g. a lone tools.json mcpproxy wrote
+		// into the cache) — no real installed source exists locally. Returning the
+		// stub here would report false "1 file" coverage AND, post-MCP-2206, would
+		// short-circuit the caller's published-source fetch fallback in Resolve(),
+		// which can fetch the REAL package source without executing it. So we treat
+		// stub-only as "not found" and let that fallback (or a tool_definitions_only
+		// degrade when fetch is disabled) take over instead.
+		r.logger.Warn("npx cache held only stub directories (no real package source); deferring to published-source fetch fallback",
+			zap.String("server", info.Name),
+			zap.String("package", pkgName),
+			zap.String("stub_path", bestMatch),
+		)
+		return nil, fmt.Errorf("npx cache for %q contained only stub directories (no real source) in %s", pkgName, npxCacheDir)
+	}
+
 	r.logger.Info("Resolved source from npx cache",
 		zap.String("server", info.Name),
 		zap.String("package", pkgName),
@@ -1220,6 +1259,38 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro
 	}, nil
 }
 
+// npxCacheLooksReal reports whether an npx cache package directory holds the
+// real installed package rather than a bare stub. Every npm package ships a
+// package.json, so its presence is the canonical marker of real source. A stub
+// directory mcpproxy creates just to hold a dumped tools.json has none. As a
+// secondary signal we also accept a directory that contains a conventional
+// build/source subdirectory (dist/lib/build/src) or any JavaScript/TypeScript
+// source file, in case an unusual package omits package.json at the cache root.
+func npxCacheLooksReal(dir string) bool {
+	if _, err := os.Stat(filepath.Join(dir, "package.json")); err == nil {
+		return true
+	}
+	for _, sub := range []string{"dist", "lib", "build", "src"} {
+		if stat, err := os.Stat(filepath.Join(dir, sub)); err == nil && stat.IsDir() {
+			return true
+		}
+	}
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		return false
+	}
+	for _, e := range entries {
+		if e.IsDir() {
+			continue
+		}
+		switch strings.ToLower(filepath.Ext(e.Name())) {
+		case ".js", ".mjs", ".cjs", ".ts":
+			return true
+		}
+	}
+	return false
+}
+
 // resolveUvxCache finds a uvx package's source in ~/.cache/uv/ or ~/.local/share/uv/tools/
 func (r *SourceResolver) resolveUvxCache(info ServerInfo) (*ResolvedSource, error) {
 	// Extract package name from args

internal/security/scanner/source_resolver_test.go

@@ -6,6 +6,7 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 
 	"go.uber.org/zap"
 )
@@ -434,6 +435,81 @@ func TestResolveNpxCache(t *testing.T) {
 	}
 }
 
+// TestResolveNpxCachePrefersRealSourceOverStub reproduces MCP-2397: when the
+// same package name exists under multiple npx cache hashes, one holding the
+// real installed source (package.json + dist) and another holding only a
+// tools.json stub that mcpproxy itself wrote, the resolver must scan the real
+// source even when the stub directory has a NEWER mtime. The old newest-mtime
+// heuristic would pick the stub and report false "1 file scanned" coverage.
+func TestResolveNpxCachePrefersRealSourceOverStub(t *testing.T) {
+	homeDir := t.TempDir()
+	t.Setenv("HOME", homeDir)
+	t.Setenv("USERPROFILE", homeDir) // Windows uses USERPROFILE
+
+	pkg := filepath.Join("node_modules", "@modelcontextprotocol", "server-everything")
+
+	// Real source under one hash: package.json + dist/*.js (older mtime).
+	realDir := filepath.Join(homeDir, ".npm", "_npx", "realhash", pkg)
+	os.MkdirAll(filepath.Join(realDir, "dist"), 0755)
+	os.WriteFile(filepath.Join(realDir, "package.json"), []byte(`{"name":"@modelcontextprotocol/server-everything"}`), 0644)
+	os.WriteFile(filepath.Join(realDir, "dist", "index.js"), []byte("// real server source"), 0644)
+
+	// Stub under a different hash: only a tools.json (newer mtime → would win
+	// under the old heuristic).
+	stubDir := filepath.Join(homeDir, ".npm", "_npx", "stubhash", pkg)
+	os.MkdirAll(stubDir, 0755)
+	os.WriteFile(filepath.Join(stubDir, "tools.json"), []byte(`{"tools":[]}`), 0644)
+
+	// Force the stub directory to be strictly newer than the real one so the
+	// legacy newest-mtime selection would choose the stub.
+	older := time.Now().Add(-2 * time.Hour)
+	newer := time.Now()
+	if err := os.Chtimes(realDir, older, older); err != nil {
+		t.Fatalf("chtimes real: %v", err)
+	}
+	if err := os.Chtimes(stubDir, newer, newer); err != nil {
+		t.Fatalf("chtimes stub: %v", err)
+	}
+
+	r := NewSourceResolver(zap.NewNop())
+	result, err := r.resolveNpxCache(ServerInfo{
+		Name:    "everything-server",
+		Command: "npx",
+		Args:    []string{"-y", "@modelcontextprotocol/server-everything"},
+	})
+	if err != nil {
+		t.Fatalf("resolveNpxCache: %v", err)
+	}
+	if result.SourceDir != realDir {
+		t.Errorf("expected real source %q, got stub %q", realDir, result.SourceDir)
+	}
+}
+
+// TestResolveNpxCacheStubOnlyReturnsError verifies the post-MCP-2206
+// interaction: when the npx cache holds ONLY a tools.json stub (no real
+// package source anywhere locally), resolveNpxCache returns an error rather
+// than the stub. This lets Resolve()'s published-source fetch fallback take
+// over and fetch the real source, instead of reporting false "1 file" coverage.
+func TestResolveNpxCacheStubOnlyReturnsError(t *testing.T) {
+	homeDir := t.TempDir()
+	t.Setenv("HOME", homeDir)
+	t.Setenv("USERPROFILE", homeDir) // Windows uses USERPROFILE
+
+	stubDir := filepath.Join(homeDir, ".npm", "_npx", "stubhash", "node_modules", "@modelcontextprotocol", "server-everything")
+	os.MkdirAll(stubDir, 0755)
+	os.WriteFile(filepath.Join(stubDir, "tools.json"), []byte(`{"tools":[]}`), 0644)
+
+	r := NewSourceResolver(zap.NewNop())
+	_, err := r.resolveNpxCache(ServerInfo{
+		Name:    "everything-server",
+		Command: "npx",
+		Args:    []string{"-y", "@modelcontextprotocol/server-everything"},
+	})
+	if err == nil {
+		t.Fatal("expected error when npx cache contains only a stub, got nil (would short-circuit the published-fetch fallback)")
+	}
+}
+
 func TestResolveNpxCacheNotFound(t *testing.T) {
 	homeDir := t.TempDir()
 	t.Setenv("HOME", homeDir)