Skip to content

fix(mcp): only derive a package from the manager's executor subcommand #34

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alexviktorov wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(mcp): only derive a package from the manager's executor subcommand #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/inventory-sources.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,16 @@ is inferred; the record falls back to the server id with `confidence=low`.
`uvx`, `uv tool run <pkg>`, and `uv run --from <pkg> ...` use the
published package name.

For `npm`/`pnpm`/`yarn`/`bun`, only the manager's package executor names a
published package: `npm exec`/`npm x`, `pnpm dlx`, `yarn dlx`, `bun x`, or
the separate `npx`/`bunx`. `exec` under pnpm, yarn, and bun runs a locally
installed binary or a shell command, so its first token is not a package.
`run <script>`, the npm lifecycle aliases (`start`/`stop`/`restart`/`test`),
bare `yarn dev` / `bun start`, `bun <file>` / `bun run <file>`, and the
`create`/`init` initializers (which resolve to a `create-<name>` package that
is intentionally not inferred) yield no package identity; the record falls
back to the server id with `confidence=low`.

Docker/OCI image refs split a pinned tag into `version`:
`hashicorp/terraform-mcp-server:0.4.0` becomes
`package_name=hashicorp/terraform-mcp-server`, `version=0.4.0`. Untagged
Expand Down
55 changes: 39 additions & 16 deletions internal/ecosystem/mcp/mcp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,8 @@ func looksUnresolvedShellVar(s string) bool {
// Supported launchers:
//
// npx / bunx -> first non-flag arg
// pnpm/yarn/bun/npm dlx|exec|x|run -> first non-flag arg past sub
// npm exec|x, pnpm dlx, yarn dlx, bun x -> first non-flag arg past sub
// (any other first token -> no spec; caller uses server id)
// uvx / pipx -> first non-flag arg
// uv / uv tool run -> first non-flag arg past sub,
// also honors --from <pkg>
Expand All @@ -427,23 +428,33 @@ func inferPackageFromArgs(cmd string, args []string) (spec, launcher string) {
}
return firstNonFlag(args, nil, npmValueTakingFlags), ""
case "pnpm", "yarn", "bun", "npm":
// These wrappers take a subcommand (dlx, exec, x, run) before the
// package. Skip the subcommand so we return the actual package
// argument rather than "dlx" / "exec" / "x". Honor
// "npm exec --package=<pkg>" / "npm exec --package <pkg>" since
// those configs name the package explicitly via flag rather than
// positional.
// We read a package identity only from the manager's package
// executor — the subcommand that fetches a published package and
// runs it (packageExecutors; npx/bunx are handled above). The
// positional after it — or --package — is the spec.
//
// Restrict the --package scan to args before "--": npm does not
// parse options past "--", so `npm exec foo -- --package @npmcli/bar`
// must resolve to foo, not @npmcli/bar.
subcommands := map[string]bool{
"dlx": true, "exec": true, "x": true, "run": true,
// Any other first token gets no package identity: return an empty
// spec and let the caller fall back to the server id at low
// confidence, like "uv run <script>" below. That stops a local-script
// launcher (`run <script>`, `npm start`, bare `yarn dev`, `bun <file>`)
// from leaking its script or file name as a package — the reported
// bug where `bun run … start` became the package "start". The same
// holds for `exec` under pnpm/yarn/bun, which runs a local bin or a
// shell command (`yarn exec node …` would otherwise become "node"),
// and for create/init, which name a create-<name> package we
// deliberately don't resolve (initializers don't launch servers).
// See mcp_test.go for these and the flag-ordering edge case.
//
// scanExplicitPackage / firstNonFlag honor "npm exec --package=<pkg>"
// and stop at "--", so `npm exec foo -- --package @npmcli/bar` -> foo.
executors := packageExecutors[bn]
if !executors[firstNonFlag(args, nil, npmValueTakingFlags)] {
return "", ""
}
if spec := scanExplicitPackage(args, subcommands); spec != "" {
if spec := scanExplicitPackage(args, executors); spec != "" {
return spec, ""
}
return firstNonFlag(args, subcommands, npmValueTakingFlags), ""
return firstNonFlag(args, executors, npmValueTakingFlags), ""
case "uvx":
return firstNonFlag(args, nil, nil), "uv"
case "uv":
Expand Down Expand Up @@ -547,8 +558,8 @@ func inferPackageFromArgs(cmd string, args []string) (spec, launcher string) {
// scanExplicitPackage looks for "--package <pkg>" / "--package=<pkg>" in
// the launcher-parsed prefix of args. The scan stops at "--" (npm/npx do
// not interpret options past it) and at the first positional non-flag
// token that is not in the skip set of recognized subcommands
// (dlx/exec/x/run). Other value-taking flags are consumed so their values
// token that is not in the skip set of recognized executor subcommands
// (packageExecutors). Other value-taking flags are consumed so their values
// are not misread as --package. Returns the explicit package spec when
// found, otherwise "".
func scanExplicitPackage(args []string, skip map[string]bool) string {
Expand Down Expand Up @@ -604,6 +615,18 @@ func firstNonFlag(args []string, skip map[string]bool, valueTaking map[string]bo
return ""
}

// packageExecutors lists, per package manager, the subcommands that fetch a
// published package and run it. Only these name a package in an MCP launch.
// `exec` qualifies under npm alone: pnpm `exec` runs a locally installed bin,
// and yarn `exec` / bun `exec` run a shell command, so their first token is a
// binary or script, not a package identity (per each manager's own CLI docs).
var packageExecutors = map[string]map[string]bool{
"npm": {"exec": true, "x": true},
"pnpm": {"dlx": true},
"yarn": {"dlx": true},
"bun": {"x": true},
}

// npmValueTakingFlags lists npm/pnpm/yarn/bun flags that consume a
// separate value argument. Without skipping past these values, a value
// like a registry URL (often credential-bearing) can be misread as the
Expand Down
117 changes: 117 additions & 0 deletions internal/ecosystem/mcp/mcp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,50 @@ func TestInferPackageFromArgs(t *testing.T) {
{"docker", []string{"run", "-e", "FOO=bar", "--name", "x", "mcp/slack"}, "mcp/slack", "docker"},
{"docker", []string{"run", "--env-file=.env", "ghcr.io/github/github-mcp-server"}, "ghcr.io/github/github-mcp-server", "docker"},
{"/usr/local/bin/docker", []string{"run", "mcp/slack"}, "mcp/slack", "docker"},
// Non-executor first tokens name no published package: the spec is
// empty and ScanConfig falls back to the server id, like `uv run
// <script>` below. The case body is identical for npm/pnpm/yarn/bun,
// so one row per distinct shape suffices.
//
// `run <script>` — incl. the reported `bun run … start` from the
// official Claude messaging plugins, which had leaked package "start".
{"bun", []string{"run", "--cwd", "/x", "--shell=bun", "--silent", "start"}, "", ""},
{"npm", []string{"run", "start"}, "", ""},
// Bare `<script>`: an npm lifecycle alias and a plain script. The gate
// returns at the first positional, so trailing args are never read.
{"npm", []string{"start"}, "", ""},
{"yarn", []string{"dev"}, "", ""},
// `bun <file>` / `bun run <file>`: bun executes the file directly. The
// path passes looksLikePackageSpec and previously leaked as the package.
{"bun", []string{"server.ts"}, "", ""},
{"bun", []string{"run", "src/index.ts"}, "", ""},
// Accepted false negative, pinned as deliberate: a bare
// `yarn <installed-bin>` is indistinguishable from a script without
// reading package.json, so it also falls back to the server id — the
// same conservative choice as `uv run`.
{"yarn", []string{"mcp-server-github"}, "", ""},
// create/init DO name a published create-<name> package, but resolving
// that is intentionally out of scope (initializers don't launch MCP
// servers); pinned so the fallback is deliberate, not an accidental drop.
{"npm", []string{"create", "vite"}, "", ""},
{"npm", []string{"init", "foo"}, "", ""},
// Known limitation: a value-taking global flag NOT in npmValueTakingFlags
// before the subcommand shifts detection, missing a genuine `exec`
// package. Does not occur in real MCP configs; documented, not desired.
{"npm", []string{"--unknownflag", "val", "exec", "real-pkg"}, "", ""},
// Value-taking flags before the subcommand are consumed by the gate, so
// a credential-bearing registry URL is neither the gate token nor the
// package.
{"npm", []string{"--registry", "https://t@reg.example.com/", "exec", "@scope/pkg"}, "@scope/pkg", ""},
// `exec` fetches a published package only under npm (`npm exec`/`npm x`).
// pnpm `exec` runs a locally installed bin; yarn `exec` and bun `exec`
// run a shell command. Their first token is a binary or script, not a
// package — and `node` is a real npm package, the same failure class
// as `start`.
{"npm", []string{"x", "left-pad"}, "left-pad", ""},
{"pnpm", []string{"exec", "node", "server.js"}, "", ""},
{"yarn", []string{"exec", "node", "dist/index.js"}, "", ""},
{"bun", []string{"exec", "node server.js"}, "", ""},
}
for _, c := range cases {
gotSpec, gotLauncher := inferPackageFromArgs(c.cmd, c.args)
Expand Down Expand Up @@ -345,6 +389,79 @@ func TestScanConfig_UVRunDirectory(t *testing.T) {
}
}

// TestScanConfig_RealWorldCorpus runs the parser end to end over command/args
// shapes representative of real MCP configurations (the modelcontextprotocol
// servers, the Claude/Cursor setup guides, the official Claude bundled-server
// plugins) plus a few script-runner shapes. It is the regression guard for
// the script-runner change: genuine package launchers (npx/uvx/docker) keep
// their package identity, while local-script launchers (`bun run … start`,
// `bun run <file>`, bare `yarn <script>`, `yarn exec <bin>`, `node <file>`,
// `npm create`) fall back to the server id at low confidence with no
// requested_spec, rather than leak a script, file, or bin token as a package.
func TestScanConfig_RealWorldCorpus(t *testing.T) {
dir := t.TempDir()
path := filepath.Join(dir, "mcp.json")
body := `{
"mcpServers": {
"seq-think": {"command":"npx","args":["-y","@modelcontextprotocol/server-sequential-thinking"]},
"omnisearch": {"command":"npx","args":["-y","mcp-omnisearch"]},
"sqlite": {"command":"uvx","args":["mcp-server-sqlite","--db-path","test.db"]},
"chronulus": {"command":"uvx","args":["chronulus-mcp"]},
"github": {"command":"docker","args":["run","-i","--rm","ghcr.io/github/github-mcp-server"]},
"local-node": {"command":"node","args":["/home/u/mcp-tools/build/index.js"]},
"discord": {"command":"bun","args":["run","--cwd","/plugins/discord","--shell=bun","--silent","start"]},
"yarn-dev": {"command":"yarn","args":["dev"]},
"scaffold": {"command":"npm","args":["create","vite"]},
"bun-file": {"command":"bun","args":["run","src/index.ts"]},
"yarn-exec": {"command":"yarn","args":["exec","node","dist/index.js"]}
}
}`
if err := os.WriteFile(path, []byte(body), 0o644); err != nil {
t.Fatal(err)
}
var out []model.Record
s := &Scanner{MaxFileSize: 1 << 20, Emit: func(r model.Record) { out = append(out, r) }}
if err := s.ScanConfig(path, model.Record{}); err != nil {
t.Fatal(err)
}
byServer := map[string]model.Record{}
for _, r := range out {
byServer[r.ServerName] = r
}
want := map[string]struct {
pkg, pm string
fallback bool // server-id fallback: confidence=low, no requested_spec
}{
// Genuine package launchers — identity preserved.
"seq-think": {"@modelcontextprotocol/server-sequential-thinking", "mcp", false},
"omnisearch": {"mcp-omnisearch", "mcp", false},
"sqlite": {"mcp-server-sqlite", "uv", false},
"chronulus": {"chronulus-mcp", "uv", false},
"github": {"ghcr.io/github/github-mcp-server", "docker", false},
// Local-script / non-package launchers — fall back to the server id.
"local-node": {"local-node", "mcp", true}, // node <file>
"discord": {"discord", "mcp", true}, // claude-plugins-official template: bun run … start
"yarn-dev": {"yarn-dev", "mcp", true}, // bare yarn <script>
"scaffold": {"scaffold", "mcp", true}, // npm create (initializer, out of scope)
"bun-file": {"bun-file", "mcp", true}, // bun run <file>
"yarn-exec": {"yarn-exec", "mcp", true}, // yarn exec runs a local bin, not a package
}
for id, w := range want {
r, ok := byServer[id]
if !ok {
t.Fatalf("%s: no record emitted", id)
}
if r.PackageName != w.pkg || r.PackageManager != w.pm {
t.Errorf("%s: got package_name=%q package_manager=%q, want %q/%q",
id, r.PackageName, r.PackageManager, w.pkg, w.pm)
}
if w.fallback && (r.Confidence != "low" || r.RequestedSpec != "") {
t.Errorf("%s: fallback record got confidence=%q requested_spec=%q, want low/empty",
id, r.Confidence, r.RequestedSpec)
}
}
}

// TestScanConfig_MalformedJSONEmitsWarn verifies that a malformed MCP
// config file is surfaced as a warn diagnostic rather than silently
// swallowed. Operators rely on diagnostics to find configs the scanner
Expand Down