Skip to content

Fix: declare @modelcontextprotocol/sdk in the workspace root manifest - #707

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/root-manifest-declare-mcp-sdk
Open

Fix: declare @modelcontextprotocol/sdk in the workspace root manifest#707
AmaadMartin wants to merge 1 commit into
mainfrom
fix/root-manifest-declare-mcp-sdk

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A

  2. Or, if no issue exists, describe the change:

Problem: tests/ belongs to no npm workspace, so the root package.json is the nearest manifest on the Node resolution path for every file under it. tests/e2e/tools/mcp/mcp_resource_server.mjs imports @modelcontextprotocol/sdk, and the root manifest does not declare it. The import resolves only through packages that other manifests declare: core's dependency, and @google/genai's optional peer dependency. No root gate can see the gap, because lint and format:check glob **/*.ts and tsc --noEmit does not include a .mjs file.

Solution: The root manifest now declares @modelcontextprotocol/sdk at ^1.26.0, the range core/package.json already uses. Mirroring core's range stops the two declarations from disagreeing, and the installed copy already satisfies it. I regenerated the lockfile with npm install; it added one line and re-resolved nothing. The whole diff is 2 files and 2 insertions.

Notes:

  • The installed tree does not change. @modelcontextprotocol/sdk@1.29.0 already satisfies ^1.26.0 and is already the single hoisted copy.
  • The lock entry keeps dev unset, because the package stays a production dependency of core. npm ci --omit=dev and the published @google/adk package are unaffected.
  • This records a coupling; it does not fix a live break. I removed @modelcontextprotocol/sdk from core/package.json in a scratch clone and reinstalled: the package is still installed, because @google/genai declares it as an optional peer at ^1.25.2. So no single-variable mutation makes the root test suite fail today. The value is that the root package now asks for what it imports, instead of relying on two indirect providers it does not own.
  • Collision check: I listed every open pull request on the staging fork and read the diff of each one that names @modelcontextprotocol or that touches a manifest. None adds @modelcontextprotocol/sdk to the root package.json. Fix: declare zod, @google-cloud/vertexai and openapi-types at the workspace root #601 (zod, @google-cloud/vertexai, openapi-types) and Fix: declare dotenv in the workspace root manifest #602 (dotenv) edit the same devDependencies block, so a textual conflict is possible, but they declare different packages. Feat: add a phantom-dependency checker and gate CI on it #450 adds @modelcontextprotocol/sdk to a phantom-dependency allowlist; if both land, that allowlist entry goes stale.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[ ] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

This change adds no executable code, so there is no new line to cover. A test that asserts the content of package.json would test npm, not ADK, and the repository has no such pattern.

I proved the existing regression signal is load-bearing instead. tests/e2e/tools/mcp/load_mcp_resource_e2e_test.ts spawns the real .mjs server as a child process. I hid the installed copy and ran that server the way the test spawns it:

mv node_modules/@modelcontextprotocol /tmp/stash
node tests/e2e/tools/mcp/mcp_resource_server.mjs
node:internal/modules/package_json_reader:314
  throw new ERR_MODULE_NOT_FOUND(packageName, fileURLToPath(base), null);

The e2e suite never reports that cause directly. The failure happens in a spawned child process, so the suite reports an MCP stdio connect failure instead.

Manual End-to-End (E2E) Tests:

npm install     # leaves the working tree clean apart from the 2 lines
npm run build   # tests/global_setup.ts imports @google/adk from dist
npx vitest run --project e2e tests/e2e/tools/mcp/load_mcp_resource_e2e_test.ts
npx vitest run --project integration tests/integration/agent_registry/agent_registry_test.ts

Results:

  • e2e: Test Files 1 passed (1), Tests 3 passed (3). This suite needs no credentials and no network. It spawns the real mcp_resource_server.mjs over stdio.
  • integration: Test Files 1 passed (1), Tests 1 passed (1). This is the other root-owned consumer, through vi.mock('@modelcontextprotocol/sdk/client/index.js').
  • npm run build: succeeded for all workspaces.
  • npm run lint and npm run format:check: pass. npx prettier --check package.json package-lock.json reports All matched files use Prettier code style!.
  • npm run ts:check: fails, and already fails on main. I ran it on the parent commit and on this commit; the output is byte-identical, so this change neither causes nor hides those errors.
  • rm -rf node_modules && npm ci, then re-run the e2e file: still Tests 3 passed (3), and git status stays clean. The lockfile is in sync with the manifests.
  • git show --stat HEAD: 2 files changed, 2 insertions(+).
  • npm ls @modelcontextprotocol/sdk now lists an unindented @modelcontextprotocol/sdk@1.29.0 under the root package, next to the existing deduped entries.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[ ] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

…nifest

tests/e2e/tools/mcp/mcp_resource_server.mjs imports
@modelcontextprotocol/sdk. That file belongs to the workspace root package,
but the root manifest does not declare the package. Node resolves the import
only through packages that other manifests declare: core's dependency, and
@google/genai's optional peer dependency.

The root gates cannot see the gap. The ts:check, lint and format:check
scripts all match *.ts only, and the importer is a .mjs file.

The root manifest now declares the package at ^1.26.0, the range core already
declares. The installed tree does not change: @modelcontextprotocol/sdk stays
at 1.29.0, and no package is added, removed or upgraded.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant