Skip to content

fix(mcp): only derive a package from executor subcommands #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 executor subcommands #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
7 changes: 7 additions & 0 deletions docs/inventory-sources.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,13 @@ 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 an executor subcommand (`dlx`/`exec`/`x`,
or the separate `npx`/`bunx`) names a published package. `run <script>`, the
npm lifecycle aliases (`start`/`stop`/`restart`/`test`), bare `yarn dev` /
`bun serve`, and the `create`/`init` initializer verbs run a local script or
initializer rather than a configured package, so no package identity is
inferred and 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
36 changes: 22 additions & 14 deletions internal/ecosystem/mcp/mcp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,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
// pnpm/yarn/bun/npm dlx|exec|x -> first non-flag arg past sub
// (run/bare <script>, create/init -> 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 @@ -367,23 +368,30 @@ 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 an executor subcommand:
// dlx/exec/x (or the separate npx/bunx 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`) from leaking
// its script name as a package — the reported bug where
// `bun run … start` became the package "start". create/init get the
// same treatment: they 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.
execSubcommands := map[string]bool{"dlx": true, "exec": true, "x": true}
if !execSubcommands[firstNonFlag(args, nil, npmValueTakingFlags)] {
return "", ""
}
if spec := scanExplicitPackage(args, subcommands); spec != "" {
if spec := scanExplicitPackage(args, execSubcommands); spec != "" {
return spec, ""
}
return firstNonFlag(args, subcommands, npmValueTakingFlags), ""
return firstNonFlag(args, execSubcommands, npmValueTakingFlags), ""
case "uvx":
return firstNonFlag(args, nil, nil), "uv"
case "uv":
Expand Down
84 changes: 84 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,29 @@ 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, a plain script, and one with
// a trailing flag (which must not be mistaken for the package).
{"npm", []string{"start"}, "", ""},
{"yarn", []string{"dev"}, "", ""},
{"yarn", []string{"build", "--port", "3000"}, "", ""},
// 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"}, "", ""},
}
for _, c := range cases {
gotSpec, gotLauncher := inferPackageFromArgs(c.cmd, c.args)
Expand Down Expand Up @@ -345,6 +368,67 @@ func TestScanConfig_UVRunDirectory(t *testing.T) {
}
}

// TestScanConfig_RealWorldCorpus runs the parser over command/args shapes
// taken from real MCP configurations (the modelcontextprotocol servers, the
// Claude/Cursor setup guides, and the official Claude bundled-server
// plugins). It is the regression guard for the script-runner change: genuine
// package launchers (npx/uvx/docker) must keep their package identity, while
// local-script launchers (`bun run … start`, bare `yarn`/`pnpm`, `node` file,
// `npm create`) must fall back to the server id rather than leak a script or
// initializer-verb 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"]}
}
}`
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 }{
// Genuine package launchers — identity preserved.
"seq-think": {"@modelcontextprotocol/server-sequential-thinking", "mcp"},
"omnisearch": {"mcp-omnisearch", "mcp"},
"sqlite": {"mcp-server-sqlite", "uv"},
"chronulus": {"chronulus-mcp", "uv"},
"github": {"ghcr.io/github/github-mcp-server", "docker"},
// Local-script / non-package launchers — fall back to the server id.
"local-node": {"local-node", "mcp"}, // node <file>
"discord": {"discord", "mcp"}, // claude-plugins-official template: bun run … start
"yarn-dev": {"yarn-dev", "mcp"}, // bare yarn <script>
"scaffold": {"scaffold", "mcp"}, // npm create (initializer, out of scope)
}
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)
}
}
}

// 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