fix(scanner): close 3 spec-parsing holes from Codex round-2 re-review

1. Flag-bypass: runnerPackageSpec parsed package flags (-p/--package/--from)
   BEFORE enforcing the dlx/x/run subcommand, so `pnpm -p start` /
   `bun -p server.ts` still produced a spec. Now the required subcommand
   keyword is enforced FIRST (skipping only leading global flags); flag
   parsing happens only on the args that follow it.

2. Argument-aware classification: isPackageRunnerCommand classified bare
   pnpm/yarn/bun/pipx as runners by name alone. It now takes args and treats
   them as runners only when the ephemeral-run keyword is actually present
   (npx/uvx/bunx remain runners by name). Decided up front, not via a later
   parser failure.

3. Version-pin miss never substitutes: when an exact pin is requested but
   absent from a local cache, resolveNpxCache, findUvxArchiveDir, the uv
   tools dir (Strategy 2), and the container npx locate script all now
   resolve NOTHING instead of falling back to a mismatched cached version
   (false coverage). The published-source fetch then gets the pinned version.
   Also fixed cleanPkg to strip PEP 440 specifiers (pkg==1.0), not just @ver.

TDD: pnpm -p start / bun -p server.ts -> no spec; isPackageRunnerCommand
arg-aware table; npx + uvx-archive + uv-tools-dir pin-miss -> not resolved;
container locate pin-miss -> empty; exact-pin hits + unpinned still resolve.
go test ./internal/security/... -race green; golangci v2 0 issues.

Related MCP-2445

Author: Dumbris

internal/security/scanner/package_fetch.go

@@ -257,9 +257,35 @@ func runnerFlagTakesValue(npm bool, flag string) bool {
 // The first remaining positional token is the package. Returns "" when no package
 // can be determined.
 func runnerPackageSpec(commandBase string, args []string) string {
-	sub := runnerSubcommand(commandBase)
-	subSkipped := sub == ""
 	npm := isNpmRunner(commandBase)
+
+	// Subcommand-style runners (pnpm/yarn `dlx`, `bun x`, `pipx run`) REQUIRE
+	// their keyword before anything is treated as a package. The keyword check
+	// runs BEFORE any flag parsing, so a package flag cannot bypass it: a bare
+	// `pnpm start`, `bun server.ts`, `pipx install foo`, or `pnpm -p start` has
+	// no keyword and resolves nothing (a local invocation, not a remote fetch —
+	// fetching the wrong token would mean false coverage + typosquat risk;
+	// MCP-2445). Leading global flags before the keyword (e.g. `pnpm --silent
+	// dlx`) are skipped. npx/uvx/bunx have no subcommand and parse from the start.
+	if sub := runnerSubcommand(commandBase); sub != "" {
+		i := 0
+		for i < len(args) {
+			arg := args[i]
+			if strings.HasPrefix(arg, "-") {
+				if i+1 < len(args) && runnerFlagTakesValue(npm, arg) {
+					i++
+				}
+				i++
+				continue
+			}
+			break // first positional token
+		}
+		if i >= len(args) || args[i] != sub {
+			return "" // first positional is not the required keyword → no package
+		}
+		args = args[i+1:] // parse only what follows the keyword
+	}
+
 	for i := 0; i < len(args); i++ {
 		arg := args[i]
 		hasNext := i+1 < len(args)
@@ -278,20 +304,6 @@ func runnerPackageSpec(commandBase string, args []string) string {
 			}
 			continue
 		}
-		// First positional token. Subcommand-style runners (pnpm/yarn/bun/pipx)
-		// REQUIRE their keyword (dlx/x/run) before the package token: a bare
-		// `pnpm start`, `bun server.ts`, or `pipx install foo` is a LOCAL
-		// invocation, NOT a remote package fetch, so it must not resolve a spec —
-		// fetching the wrong token would mean false coverage and typosquat risk
-		// (MCP-2445 re-review). Once the keyword is consumed the next positional
-		// is the package.
-		if !subSkipped {
-			if arg == sub {
-				subSkipped = true
-				continue
-			}
-			return ""
-		}
 		return arg
 	}
 	return ""

internal/security/scanner/package_fetch_test.go

@@ -73,6 +73,13 @@ func TestRunnerPackageSpec(t *testing.T) {
 		{"bun run dev (local script)", "bun", []string{"run", "dev"}, ""},
 		{"yarn build (local script)", "yarn", []string{"build"}, ""},
 		{"pipx install foo (not ephemeral run)", "pipx", []string{"install", "foo"}, ""},
+		// The subcommand keyword is enforced BEFORE flag parsing, so a package
+		// flag cannot bypass it (MCP-2445 round-2 hole #1).
+		{"pnpm -p start (flag bypass)", "pnpm", []string{"-p", "start"}, ""},
+		{"bun -p server.ts (flag bypass)", "bun", []string{"-p", "server.ts"}, ""},
+		{"pnpm --package x (no dlx)", "pnpm", []string{"--package", "x", "start"}, ""},
+		// Leading global flags before the keyword are fine; keyword still required.
+		{"pnpm --silent dlx pkg", "pnpm", []string{"--silent", "dlx", "pkg"}, "pkg"},
 		// Degenerate inputs.
 		{"npx flag only", "npx", []string{"-y"}, ""},
 		{"nil", "npx", nil, ""},

internal/security/scanner/source_resolver.go

@@ -174,7 +174,7 @@ func (r *SourceResolver) Resolve(ctx context.Context, info ServerInfo) (*Resolve
 	// a server like `npx @modelcontextprotocol/server-filesystem /tmp/data`
 	// from picking up the user's data dir (`/tmp/data`) as the server
 	// source — the arg is the filesystem server's allowed root, not code.
-	if info.Command != "" && isPackageRunnerCommand(info.Command) {
+	if info.Command != "" && isPackageRunnerCommand(info.Command, info.Args) {
 		if resolved, err := r.resolveFromPackageCache(ctx, info); err == nil {
 			return resolved, nil
 		} else {
@@ -256,7 +256,7 @@ func (r *SourceResolver) Resolve(ctx context.Context, info ServerInfo) (*Resolve
 
 	// Last resort: try the package cache for any other command (e.g. the user
 	// has an absolute path to node_modules that we didn't match above).
-	if info.Command != "" && !isPackageRunnerCommand(info.Command) {
+	if info.Command != "" && !isPackageRunnerCommand(info.Command, info.Args) {
 		if resolved, err := r.resolveFromPackageCache(ctx, info); err == nil {
 			return resolved, nil
 		}
@@ -267,7 +267,7 @@ func (r *SourceResolver) Resolve(ctx context.Context, info ServerInfo) (*Resolve
 	// a quarantined-on-add server is never run locally, so the local cache above
 	// always misses (MCP-2206). Fetching real source lets the AI + supply-chain
 	// scanners run instead of degrading to tool_definitions_only.
-	if r.fetchPackageSource && info.Command != "" && isPackageRunnerCommand(info.Command) {
+	if r.fetchPackageSource && info.Command != "" && isPackageRunnerCommand(info.Command, info.Args) {
 		if resolved, err := r.resolveFromPackageFetch(ctx, info); err == nil {
 			return resolved, nil
 		} else {
@@ -286,15 +286,22 @@ func (r *SourceResolver) Resolve(ctx context.Context, info ServerInfo) (*Resolve
 // a remote registry rather than running local source code. For these, the
 // server source lives in the package manager's cache, not in any positional
 // argument.
-func isPackageRunnerCommand(command string) bool {
+//
+// Classification is ARGUMENT-AWARE for subcommand-style runners (MCP-2445):
+// npx/uvx/bunx always run a remote package by name, but pnpm/yarn/bun/pipx are
+// general multi-command tools — they are runners ONLY when their ephemeral-run
+// keyword (`dlx`/`x`/`run`) is actually present. Classifying `pnpm start` or
+// `bun server.ts` as a runner by name alone would route a local invocation into
+// the package cache/fetch path and scan the wrong token. We decide this up front
+// rather than relying on a later parser failure.
+func isPackageRunnerCommand(command string, args []string) bool {
 	base := strings.ToLower(filepath.Base(command))
 	switch base {
-	// pnpm/yarn are runners only in their `dlx` form; that is the only way an MCP
-	// server is launched from them, and resolveFromPackageFetch dispatches pnpm to
-	// the npm fetch path. Including them here lets that fetch fallback actually run
-	// (previously pnpm was dispatched but never gated true here — a dead branch).
-	case "npx", "uvx", "pipx", "bunx", "bun", "pnpm", "yarn":
+	case "npx", "uvx", "bunx":
 		return true
+	case "pipx", "pnpm", "yarn", "bun":
+		// Runner only when the ephemeral-run keyword yields a resolvable package.
+		return runnerPackageSpec(base, args) != ""
 	}
 	return false
 }
@@ -792,19 +799,20 @@ func uvxTargetPackage(info ServerInfo) string {
 // embedding. Factored out so the shell logic is unit-testable on the host.
 func npxContainerLocateScript(npxGlobRoot, pkg, version string) string {
 	pkgEsc := strings.ReplaceAll(pkg, "'", `'\''`)
-	verClause := ""
 	if version != "" {
-		verEsc := strings.ReplaceAll(version, "'", `'\''`)
-		// The .dist-info-free npm case: match the version inside package.json. The
+		// Pinned: return ONLY a bucket whose package.json declares that exact
+		// version. A pin-miss returns nothing — never a mismatched version, which
+		// would scan the wrong code and report false coverage (MCP-2445). The
 		// pattern tolerates arbitrary JSON spacing around the colon.
-		verClause = "if grep -Eq '\"version\"[[:space:]]*:[[:space:]]*\"" + verEsc + "\"' \"$d/package.json\" 2>/dev/null; then printf '%s\\n' \"$d\"; exit 0; fi; "
-	}
-	return "first=''; for d in " + npxGlobRoot + "/*/node_modules/'" + pkgEsc + "'; do " +
-		"[ -d \"$d\" ] || continue; " +
-		"[ -z \"$first\" ] && first=\"$d\"; " +
-		verClause +
-		"done; " +
-		"[ -n \"$first\" ] && printf '%s\\n' \"$first\""
+		verEsc := strings.ReplaceAll(version, "'", `'\''`)
+		return "for d in " + npxGlobRoot + "/*/node_modules/'" + pkgEsc + "'; do " +
+			"[ -d \"$d\" ] || continue; " +
+			"if grep -Eq '\"version\"[[:space:]]*:[[:space:]]*\"" + verEsc + "\"' \"$d/package.json\" 2>/dev/null; then printf '%s\\n' \"$d\"; exit 0; fi; " +
+			"done"
+	}
+	// Unpinned: first matching bucket.
+	return "for d in " + npxGlobRoot + "/*/node_modules/'" + pkgEsc + "'; do " +
+		"[ -d \"$d\" ] || continue; printf '%s\\n' \"$d\"; exit 0; done"
 }
 
 // findContainerTargetDir locates the target package's directory inside a
@@ -951,7 +959,7 @@ func (r *SourceResolver) ResolveFullSource(ctx context.Context, info ServerInfo)
 	// source without executing it (MCP-2206). Same rationale as Pass 1 — without
 	// a local container or cache, Pass 2 supply-chain scanning would otherwise
 	// have nothing to analyze for npx/uvx servers.
-	if r.fetchPackageSource && info.Command != "" && isPackageRunnerCommand(info.Command) {
+	if r.fetchPackageSource && info.Command != "" && isPackageRunnerCommand(info.Command, info.Args) {
 		if resolved, err := r.resolveFromPackageFetch(ctx, info); err == nil {
 			return resolved, nil
 		}
@@ -1347,6 +1355,19 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro
 		return nil, fmt.Errorf("package %q not found in npx cache (%s)", pkgName, npxCacheDir)
 	}
 
+	if wantVersion != "" && !bestVerMatch {
+		// An exact version was pinned but no cached candidate declares it. Do NOT
+		// substitute a different cached version — scanning the wrong code reports
+		// false coverage (MCP-2445). Defer to the published-source fetch fallback,
+		// which fetches the PINNED version, or degrade to tool_definitions_only.
+		r.logger.Warn("npx cache has the package but not the pinned version; deferring to published-source fetch",
+			zap.String("server", info.Name),
+			zap.String("package", pkgName),
+			zap.String("pinned_version", wantVersion),
+		)
+		return nil, fmt.Errorf("npx cache for %q has no entry matching pinned version %q (avoiding mismatched-version scan)", pkgName, wantVersion)
+	}
+
 	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
@@ -1443,6 +1464,13 @@ func (r *SourceResolver) resolveUvxCache(info ServerInfo) (*ResolvedSource, erro
 	// Check if it's a git URL: git+https://github.com/...
 	isGitURL := strings.HasPrefix(pkgName, "git+")
 
+	// Exact version pin (if any). Only meaningful for registry packages — git
+	// specs pin via the URL ref, not a PEP 440 version.
+	wantVersion := ""
+	if !isGitURL {
+		_, wantVersion = parsePackageSpec(pkgName)
+	}
+
 	homeDir, err := os.UserHomeDir()
 	if err != nil {
 		return nil, fmt.Errorf("cannot determine home directory: %w", err)
@@ -1474,33 +1502,48 @@ func (r *SourceResolver) resolveUvxCache(info ServerInfo) (*ResolvedSource, erro
 		}
 	}
 
-	// Strategy 2: UV tools directory (for regular packages)
-	// Strip version: package@version → package
+	// Strategy 2: UV tools directory (for regular packages). Reduce the spec to a
+	// bare distribution name: strip a git+ URL to its repo, otherwise strip the
+	// `@version` form AND any PEP 440 specifier (`==`, `>=`, extras …) — the latter
+	// was previously missed, so a `pkg==1.0` spec produced a bogus `tools/pkg==1.0`
+	// path that never matched.
 	cleanPkg := pkgName
-	if idx := strings.LastIndex(cleanPkg, "@"); idx > 0 {
-		cleanPkg = cleanPkg[:idx]
-	}
-	// Also strip git+ prefix and URL
 	if strings.HasPrefix(cleanPkg, "git+") {
 		// Extract package name from URL: git+https://github.com/org/repo → repo
 		parts := strings.Split(cleanPkg, "/")
 		if len(parts) > 0 {
 			cleanPkg = parts[len(parts)-1]
 		}
+	} else {
+		if idx := strings.LastIndex(cleanPkg, "@"); idx > 0 {
+			cleanPkg = cleanPkg[:idx]
+		}
+		cleanPkg = stripPkgVersion(cleanPkg)
 	}
 
 	toolsDir := filepath.Join(homeDir, ".local", "share", "uv", "tools", cleanPkg)
 	if stat, err := os.Stat(toolsDir); err == nil && stat.IsDir() {
-		r.logger.Info("Resolved source from UV tools directory",
+		// When a version is pinned, only accept the tools dir if it actually holds
+		// that version — otherwise fall through so the published-source fetch gets
+		// the PINNED version instead of scanning whatever was `uv tool install`-ed
+		// (MCP-2445: never substitute a mismatched version).
+		if wantVersion == "" || uvDirHasVersion(toolsDir, stripPkgVersion(cleanPkg), wantVersion) {
+			r.logger.Info("Resolved source from UV tools directory",
+				zap.String("server", info.Name),
+				zap.String("package", cleanPkg),
+				zap.String("path", toolsDir),
+			)
+			return &ResolvedSource{
+				SourceDir: toolsDir,
+				Method:    "uvx_cache",
+				Cleanup:   func() {},
+			}, nil
+		}
+		r.logger.Warn("UV tools dir present but not the pinned version; deferring to archive/published-source fetch",
 			zap.String("server", info.Name),
 			zap.String("package", cleanPkg),
-			zap.String("path", toolsDir),
+			zap.String("pinned_version", wantVersion),
 		)
-		return &ResolvedSource{
-			SourceDir: toolsDir,
-			Method:    "uvx_cache",
-			Cleanup:   func() {},
-		}, nil
 	}
 
 	// Strategy 3: ephemeral uvx archive cache (~/.cache/uv/archive-v0/<hash>/).
@@ -1516,9 +1559,8 @@ func (r *SourceResolver) resolveUvxCache(info ServerInfo) (*ResolvedSource, erro
 	// not archive-v0, so they are skipped here.
 	if !isGitURL {
 		archiveRoot := filepath.Join(homeDir, ".cache", "uv", "archive-v0")
-		// Honor an exact version pin: prefer the archive entry whose .dist-info
-		// declares the pinned version over a newer-but-mismatched one (MCP-2445).
-		_, wantVersion := parsePackageSpec(pkgName)
+		// Honor an exact version pin: select the archive entry whose .dist-info
+		// declares the pinned version; a pin-miss resolves nothing (MCP-2445).
 		if dir, ok := findUvxArchiveDir(archiveRoot, stripPkgVersion(cleanPkg), wantVersion); ok {
 			r.logger.Info("Resolved source from UV archive cache",
 				zap.String("server", info.Name),
@@ -1635,9 +1677,36 @@ func findUvxArchiveDir(archiveRoot, pkg, wantVersion string) (string, bool) {
 	if best == "" {
 		return "", false
 	}
+	if wantVersion != "" && !bestVerMatch {
+		// Pinned version requested but no archive entry declares it. Do not
+		// substitute a different cached version (false coverage) — report not found
+		// so the caller fetches the pinned version or degrades (MCP-2445).
+		return "", false
+	}
 	return best, true
 }
 
+// uvDirHasVersion reports whether a uv venv-style directory (a `uv tool install`
+// tools dir) holds distribution pkg at exactly version. It looks for a matching
+// `<dist>-<version>.dist-info` under any `lib/python*/site-packages`. Used to keep
+// a version pin honest against the persistent tools dir.
+func uvDirHasVersion(root, pkg, version string) bool {
+	if version == "" {
+		return false
+	}
+	norm := normalizeDistName(pkg)
+	if norm == "" {
+		return false
+	}
+	sps, _ := filepath.Glob(filepath.Join(root, "lib", "python*", "site-packages"))
+	for _, d := range sps {
+		if _, vm := distInfoMatch(d, norm, version); vm {
+			return true
+		}
+	}
+	return false
+}
+
 // distInfoMatch reports whether dir directly contains a `<name>-<version>.dist-info`
 // directory whose normalized distribution name equals norm (nameMatch), and — when
 // wantVersion is non-empty — whether that directory's version equals wantVersion