Fix npm dependency-confusion bug in MCP server default command

.github/workflows/codeql.yml

@@ -24,17 +24,17 @@ jobs:
 
     steps:
     - name: Checkout repository
-      uses: actions/checkout@v4
+      uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
 
     - name: Initialize CodeQL
-      uses: github/codeql-action/init@v3
+      uses: github/codeql-action/init@db488ddef3bf6cb639b32c2e9a7c0a7ea8271d28 # v4.37.8
       with:
         languages: ${{ matrix.language }}
 
     - name: Autobuild
-      uses: github/codeql-action/autobuild@v3
+      uses: github/codeql-action/autobuild@db488ddef3bf6cb639b32c2e9a7c0a7ea8271d28 # v4.37.8
 
     - name: Perform CodeQL Analysis
-      uses: github/codeql-action/analyze@v3
+      uses: github/codeql-action/analyze@db488ddef3bf6cb639b32c2e9a7c0a7ea8271d28 # v4.37.8
       with:
         category: "/language:${{ matrix.language }}"

README.md

@@ -283,8 +283,9 @@ Add it to your MCP client's config (for Claude Desktop, `claude_desktop_config.j
 }
 ```
 
-The server exposes one tool, `run`, that shells out to the published `graphkeeper` npm binary with
-the given subcommand and arguments plus `--json`, and returns the parsed JSON result:
+The server exposes one tool, `run`, that shells out to the published `graphkeeper-cli` npm package
+(via `npx graphkeeper-cli`) with the given subcommand and arguments plus `--json`, and returns the
+parsed JSON result:
 
 ```
 run(["build", "."])

python/src/graphkeeper/mcp_server.py

@@ -6,13 +6,20 @@
 underlying CLI with --json appended, parses the JSON stdout, and returns it
 as the tool result.
 
-Deliberately shells out to the Node/TypeScript CLI (`npx graphkeeper`)
+Deliberately shells out to the Node/TypeScript CLI (`npx graphkeeper-cli`)
 rather than calling into this same Python package's own native
 `graphkeeper.cli` module. Per the template, the MCP wrapper's implementation
 language is independent of the CLI's: this keeps one wrapper shape reusable
 across the whole portfolio regardless of whether a given repo's CLI is Node
-or Python, and it exercises the actual published `graphkeeper` npm binary
-end to end rather than a parallel code path.
+or Python, and it exercises the actual published `graphkeeper-cli` npm
+package end to end rather than a parallel code path.
+
+NOTE ON THE PACKAGE NAME: the published npm package is `graphkeeper-cli`
+(https://www.npmjs.com/package/graphkeeper-cli), not `graphkeeper`. A
+completely unrelated, unaudited third-party package is published under the
+bare name `graphkeeper` by a different maintainer -- `npx graphkeeper`
+would silently install and run that package instead of this project's CLI
+(a dependency-confusion trap). Always use the `-cli` suffix here.
 
 stdout is reserved for MCP's JSON-RPC framing, so anything this module
 logs goes to stderr.
@@ -27,17 +34,23 @@
 
 from mcp.server import MCPServer
 
-# Local-test override: point at a built dist/cli.js instead of `npx graphkeeper`.
+# Local-test override: point at a built dist/cli.js instead of `npx graphkeeper-cli`.
 # The npm package may not be globally linked on a dev machine, so testing
 # this wrapper against a repo checkout needs a direct `node <path>` command.
-# Production default (no env var set) is `npx graphkeeper`.
+# Production default (no env var set) is `npx graphkeeper-cli`.
+#
+# IMPORTANT: the published npm package name is `graphkeeper-cli`, NOT
+# `graphkeeper`. `graphkeeper` (no suffix) is an unrelated third-party
+# package from a different publisher -- using the bare name here would be a
+# dependency-confusion bug, silently running someone else's code instead of
+# this project's CLI. Do not drop the `-cli` suffix.
 _LOCAL_CLI_JS = os.environ.get("GRAPHKEEPER_CLI_JS")
 
 
 def _base_command() -> list[str]:
     if _LOCAL_CLI_JS:
         return ["node", _LOCAL_CLI_JS]
-    return ["npx", "graphkeeper"]
+    return ["npx", "graphkeeper-cli"]
 
 
 _TOOL_DESCRIPTION = """Run the GraphKeeper CLI to mine a local git repo's commit history for file-level co-change patterns and query the resulting knowledge graph. Call this when an agent needs to know which files tend to change together in a codebase (e.g. before editing a file, to see what else usually needs to change alongside it) or which functions call/are called by a given symbol.

python/src/graphkeeper/query.py

@@ -13,13 +13,48 @@
 from .types import CallsQueryResult, CoChangeQueryResult, CoChangeResultRow, GraphKeeperStore, GraphifyNode
 
 
+def _is_absolute(path_str: str) -> bool:
+    """Cross-platform absolute-path check matching the semantics of Node's
+    `path.isAbsolute`, which the TypeScript original (src/query.ts) relies
+    on.
+
+    On Windows, `os.path.isabs` disagrees with Node here for *root-relative*
+    paths that have no drive letter (e.g. "/repo/src/a.py" or
+    "\\repo\\src\\a.py"): Node's `path.isAbsolute` treats a leading path
+    separator as absolute (resolved against the current drive), but
+    `os.path.isabs` returns False for it, silently skipping relativization
+    for exactly the POSIX-style absolute paths an MCP client is likely to
+    pass. On POSIX this matches `os.path.isabs` exactly (leading "/").
+    """
+    if not path_str:
+        return False
+    if os.name == "nt":
+        first = path_str[0]
+        if first in ("\\", "/"):
+            return True
+        if len(path_str) > 2 and path_str[1] == ":" and path_str[2] in ("\\", "/"):
+            return True
+        return False
+    return path_str.startswith("/")
+
+
 def normalize_file_arg(store: GraphKeeperStore, file: str) -> str:
     """Normalizes a user-supplied file argument to the repo-relative form
     GraphKeeper stores paths in."""
     normalized = file
-    if os.path.isabs(normalized):
-        rel = os.path.relpath(normalized, store.repo_path)
-        if not rel.startswith(".."):
+    if _is_absolute(normalized):
+        try:
+            rel = os.path.relpath(normalized, store.repo_path)
+        except ValueError:
+            # Windows-only: os.path.relpath raises when `normalized` and
+            # `store.repo_path` are on different drives (e.g. repo on C:,
+            # file arg on D:) -- there is no relative path between them.
+            # The TypeScript original (path.relative) doesn't throw in this
+            # case; it just returns the absolute target path unchanged,
+            # which also fails the `not rel.startswith("..")` check below.
+            # Mirror that by leaving `normalized` as-is instead of crashing.
+            rel = None
+        if rel is not None and not rel.startswith(".."):
             normalized = rel
     normalized = re.sub(r"^\./", "", normalized)
     return normalized.replace(os.sep, "/")

python/tests/test_mcp_server.py

@@ -0,0 +1,84 @@
+"""Regression tests guarding against the dependency-confusion bug where the
+MCP wrapper shelled out to `npx graphkeeper` (no `-cli` suffix) instead of
+the actually-published `npx graphkeeper-cli`. `graphkeeper` (bare name) is
+an unrelated third-party npm package from a different publisher; running it
+instead of this project's CLI would silently execute unaudited code.
+
+These tests only exercise this module's own `_base_command()` logic, not
+the MCP SDK itself, so they stub `mcp.server.MCPServer` when it isn't
+importable rather than skipping outright -- this project's pyproject.toml
+requires `mcp[cli]>=2.0.0`, and some local/shared environments may still
+have an older `mcp` pinned for unrelated projects that predates that name,
+which shouldn't stop this dependency-confusion regression test from running.
+"""
+from __future__ import annotations
+
+import importlib
+
+import pytest
+
+pytest.importorskip("mcp", reason="optional `mcp` extra not installed")
+
+
+def _ensure_mcpserver_importable() -> None:
+    try:
+        from mcp.server import MCPServer  # noqa: F401
+
+        return
+    except ImportError:
+        pass
+
+    import mcp.server as real_server_module
+
+    class _StubMCPServer:
+        """Minimal stand-in with just enough surface for mcp_server.py's
+        module-level `mcp = MCPServer(name=...)` and `@mcp.tool(...)` to
+        execute without error."""
+
+        def __init__(self, name: str) -> None:
+            self.name = name
+
+        def tool(self, description: str = ""):
+            def decorator(func):
+                return func
+
+            return decorator
+
+    real_server_module.MCPServer = _StubMCPServer
+
+
+_ensure_mcpserver_importable()
+
+
+@pytest.fixture()
+def mcp_server_module(monkeypatch):
+    monkeypatch.delenv("GRAPHKEEPER_CLI_JS", raising=False)
+    import graphkeeper.mcp_server as module
+
+    return importlib.reload(module)
+
+
+class TestBaseCommand:
+    def test_default_command_uses_published_cli_package_name(self, mcp_server_module):
+        # The published npm package is `graphkeeper-cli`, not `graphkeeper`.
+        # `npx graphkeeper` (missing the `-cli` suffix) resolves to a
+        # completely different, unrelated npm package -- this must never
+        # be the default.
+        assert mcp_server_module._base_command() == ["npx", "graphkeeper-cli"]
+
+    def test_default_command_never_omits_cli_suffix(self, mcp_server_module):
+        command = mcp_server_module._base_command()
+        assert "graphkeeper" not in command, (
+            "the bare `graphkeeper` npm package is an unrelated third-party "
+            "package; the CLI must always be invoked as `graphkeeper-cli`"
+        )
+
+    def test_local_override_still_takes_precedence(self, mcp_server_module, monkeypatch, tmp_path):
+        cli_js = tmp_path / "cli.js"
+        monkeypatch.setenv("GRAPHKEEPER_CLI_JS", str(cli_js))
+        module = importlib.reload(mcp_server_module)
+        try:
+            assert module._base_command() == ["node", str(cli_js)]
+        finally:
+            monkeypatch.delenv("GRAPHKEEPER_CLI_JS", raising=False)
+            importlib.reload(module)

python/tests/test_query.py

@@ -1,10 +1,12 @@
 """Ported from test/query.test.ts."""
 from __future__ import annotations
 
+import os
 from datetime import datetime, timezone
 from typing import Optional
+from unittest.mock import patch
 
-from graphkeeper.query import find_graphify_node, normalize_file_arg, query_calls, query_co_change
+from graphkeeper.query import _is_absolute, find_graphify_node, normalize_file_arg, query_calls, query_co_change
 from graphkeeper.types import CoChangeEdge, GraphifyEnrichment, GraphKeeperStore
 
 
@@ -38,6 +40,40 @@ def test_relativizes_absolute_path_under_repo_path(self):
     def test_leaves_absolute_path_outside_repo_path_unchanged(self):
         assert normalize_file_arg(_make_store(repo_path="/repo"), "/other/src/a.py") == "/other/src/a.py"
 
+    def test_does_not_crash_when_relpath_cannot_compute_a_relative_path(self):
+        # Windows-only in practice: os.path.relpath raises ValueError when
+        # the two paths are on different drives (e.g. repo on C:, file arg
+        # on D:), since there is no relative path between them. The
+        # TypeScript original (path.relative) doesn't throw in that case --
+        # it just returns the absolute target path unchanged. Force
+        # `_is_absolute` and mock os.path.relpath so this regression test
+        # exercises that branch on every platform, not just Windows, and
+        # use forward slashes throughout so the os.sep normalization at the
+        # end of the function is a no-op regardless of host OS.
+        with patch("graphkeeper.query._is_absolute", return_value=True), patch.object(
+            os.path, "relpath", side_effect=ValueError("path is on mount 'D:', start on mount 'C:'")
+        ):
+            result = normalize_file_arg(_make_store(repo_path="C:/repo"), "D:/other/file.py")
+        assert result == "D:/other/file.py"
+
+
+class TestIsAbsolute:
+    """`_is_absolute` exists because `os.path.isabs` disagrees with Node's
+    `path.isAbsolute` (which the TS original uses) on Windows for
+    root-relative paths with no drive letter -- e.g. `os.path.isabs`
+    returns False for "/repo/src/a.py" on Windows, so the un-fixed Python
+    port silently skipped relativizing exactly this kind of path."""
+
+    def test_posix_style_root_relative_path_is_absolute(self):
+        assert _is_absolute("/repo/src/a.py") is True
+
+    def test_relative_path_is_not_absolute(self):
+        assert _is_absolute("src/a.py") is False
+        assert _is_absolute("./src/a.py") is False
+
+    def test_empty_string_is_not_absolute(self):
+        assert _is_absolute("") is False
+
 
 class TestQueryCoChange:
     def test_finds_co_change_partners_ranked_by_count(self):