fix(mcp): expose every registered tool and fix SDK subpath resolution

Two related fixes for `gsd --mode mcp` that the audit missed on first pass:

1. Tool inventory — session.agent.state.tools was the *active* subset, not
   the full registry. Before this change, MCP clients connected to GSD saw
   63 tools and four built-ins were silently missing: `find`, `grep`, `ls`,
   and `hashline_edit`. After: 67 tools, matching the full _toolRegistry.
   Fix: call session.getAllTools() + session.setActiveToolsByName() before
   starting the MCP transport so every registered tool is active for the
   lifetime of the MCP session.

2. SDK subpath resolution — the #3603 createRequire workaround no longer
   works with @modelcontextprotocol/sdk 1.27.x + current Node. The
   wildcard export ./* → ./dist/cjs/* does NOT auto-append `.js`, and
   _require.resolve fails with "Cannot find module .../server/stdio".
   End-to-end handshake was actually broken in src/mcp-server.ts even
   before my earlier F5 change. Fix: use explicit `.js` suffixes on
   every subpath import (server/index.js, server/stdio.js, types.js),
   matching the convention already in use by packages/mcp-server/.

The regression test is rewritten to enforce the `.js`-suffix convention
and reject any bare subpath or lingering createRequire resolution.

Verified end-to-end via raw JSON-RPC against `gsd --mode mcp --bare`:
  BEFORE_COUNT=63
  AFTER_COUNT=67
  diff: +find +grep +hashline_edit +ls

Test sweep: 76 tests pass across mcp-createRequire, stream-adapter,
mcp-server, workflow-tools.

https://claude.ai/code/session_0174sYny3VvdwYTdCNTmY4Do
This commit is contained in:
Claude 2026-04-13 01:34:51 +00:00
parent dc489f0a07
commit 1eb357ca46
3 changed files with 55 additions and 19 deletions

View file

@ -546,6 +546,17 @@ if (isPrintMode) {
if (mode === 'mcp') {
printStartupTimings()
const { startMcpServer } = await import('./mcp-server.js')
// Activate every registered tool before starting the MCP transport.
// `session.agent.state.tools` is the *active* subset, not the full
// registry — if we expose only the active set, extension-registered
// tools (gsd workflow, browser-tools, mac-tools, search-the-web, …)
// are invisible to MCP clients. Flipping the active set to every
// known tool name makes `state.tools` mirror the full registry for
// this MCP session, which is what an external client expects.
const allToolNames = session.getAllTools().map((t) => t.name)
session.setActiveToolsByName(allToolNames)
await startMcpServer({
tools: session.agent.state.tools ?? [],
version: process.env.GSD_VERSION || '0.0.0',

View file

@ -16,13 +16,18 @@ export interface McpToolDef {
}>
}
// MCP SDK subpath imports use wildcard exports (./*) that NodeNext resolves
// at runtime but TypeScript cannot statically type-check. We construct the
// specifiers dynamically so tsc treats them as `any`.
// MCP SDK subpath imports use wildcard exports (./*) in @modelcontextprotocol/sdk's
// package.json export map. The wildcard maps "./foo" → "./dist/cjs/foo" (no .js
// suffix), so bare subpath specifiers like `${MCP_PKG}/server/stdio` resolve to
// a non-existent file. Historically the workaround (#3603) used createRequire so
// the CJS resolver could auto-append `.js`; that no longer works with current
// Node + SDK releases (#3914) — `_require.resolve` also fails with
// "Cannot find module .../dist/cjs/server/stdio".
//
// Use explicit .js subpaths for modules that are loaded dynamically at runtime.
// Recent Node / SDK combinations do not reliably resolve the extensionless
// wildcard targets for `server/stdio` and `types` (#3914).
// The reliable convention (matching packages/mcp-server/{server,cli}.ts) is to
// write the `.js` suffix explicitly on every wildcard subpath. Specifiers are
// built via a template string so TypeScript's NodeNext resolver treats them as
// `any` and skips static checking.
const MCP_PKG = '@modelcontextprotocol/sdk'
/**
@ -45,7 +50,7 @@ export async function startMcpServer(options: {
}): Promise<void> {
const { tools, version = '0.0.0' } = options
const serverMod = await import(`${MCP_PKG}/server`)
const serverMod = await import(`${MCP_PKG}/server/index.js`)
const stdioMod = await import(`${MCP_PKG}/server/stdio.js`)
const typesMod = await import(`${MCP_PKG}/types.js`)

View file

@ -1,9 +1,17 @@
/**
* Regression test for #3914 MCP server uses explicit .js SDK subpaths.
* Regression test for #3603 / #3914 MCP server subpath imports.
*
* Extensionless wildcard exports for `server/stdio` and `types` do not resolve
* reliably across current Node / SDK combinations. The runtime import strings
* must include `.js`.
* @modelcontextprotocol/sdk's package.json exports map uses a wildcard
* `./*` `./dist/cjs/*` with no `.js` suffix, so bare subpath specifiers
* like `@modelcontextprotocol/sdk/server/stdio` resolve to a file that
* doesn't exist. Historically the workaround used `createRequire` so the
* CJS resolver auto-appended `.js`; that no longer works with current
* Node + SDK versions (#3914).
*
* The reliable convention (used in packages/mcp-server/{server,cli}.ts)
* is to write the `.js` suffix explicitly on every subpath import. This
* test locks that convention in so regressions can't silently reintroduce
* the bare subpath form or the broken createRequire-based resolution.
*/
import { describe, test } from 'node:test';
@ -17,19 +25,31 @@ const __dirname = dirname(__filename);
const source = readFileSync(join(__dirname, '..', 'mcp-server.ts'), 'utf-8');
describe('MCP server SDK subpath imports (#3914)', () => {
test('server/stdio import uses explicit .js subpath', () => {
assert.match(source, /await import\(`\$\{MCP_PKG\}\/server\/stdio\.js`\)/,
'server/stdio import should include the .js suffix');
describe('MCP server SDK subpath imports (#3603 / #3914)', () => {
test('server/index.js subpath is imported with explicit .js suffix', () => {
assert.match(source, /await import\(`\$\{MCP_PKG\}\/server\/index\.js`\)/,
'server import must use `${MCP_PKG}/server/index.js` to satisfy the wildcard export map');
});
test('types import uses explicit .js subpath', () => {
test('server/stdio.js subpath is imported with explicit .js suffix', () => {
assert.match(source, /await import\(`\$\{MCP_PKG\}\/server\/stdio\.js`\)/,
'stdio import must use `${MCP_PKG}/server/stdio.js`');
});
test('types.js subpath is imported with explicit .js suffix', () => {
assert.match(source, /await import\(`\$\{MCP_PKG\}\/types\.js`\)/,
'types import should include the .js suffix');
'types import must use `${MCP_PKG}/types.js`');
});
test('legacy createRequire-based resolution is gone', () => {
assert.doesNotMatch(source, /createRequire|_require\.resolve/,
'legacy createRequire-based subpath resolution should not remain');
// Only flag actual code, not the comment that explains the history.
// The import statement, variable declaration, and `_require.resolve(` call
// sites are the real regression surfaces.
assert.doesNotMatch(source, /^\s*import\s*\{\s*createRequire\s*\}\s*from/m,
'createRequire should not be imported from node:module');
assert.doesNotMatch(source, /^\s*const\s+_require\s*=\s*createRequire/m,
'_require helper should not be created');
assert.doesNotMatch(source, /_require\.resolve\(/,
'_require.resolve should not be used for subpath resolution');
});
});