Skip to content

feat(upstreams): mount upstream MCP servers as namespaced child processes - #17

Open
dobrinyonkov wants to merge 5 commits into
mainfrom
feat/upstream-mounting
Open

feat(upstreams): mount upstream MCP servers as namespaced child processes#17
dobrinyonkov wants to merge 5 commits into
mainfrom
feat/upstream-mounting

Conversation

@dobrinyonkov

@dobrinyonkov dobrinyonkov commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes: #16

Adds an opt-in upstream-mounting layer so this server can spawn another MCP server as a child over stdio and re-publish its tools, resources, and prompts under a configurable name prefix.

The first registered upstream is @ui5/webcomponents-react-mcp under the react prefix. Net effect for users: one install of @ui5/webcomponents-mcp-server now also surfaces the React MCP's tools as react_*. No breaking change to the existing native surface.

Why

The Web Components team ships this server. The React team ships their own. Future framework teams (Vue, Angular, …) will ship more. Customers shouldn't need to wire up N servers to get full UI5 webcomponents coverage - they install one MCP and the rest follows.

Adding a future framework MCP becomes a one-entry addition to src/upstreams/registry.ts plus the new package as a runtime dependency.

What changes

Area Change
src/upstreams/registry.ts UPSTREAMS list (one entry today, React).
src/upstreams/mount.ts Spawns each upstream via StdioClientTransport, lists capabilities, converts JSON Schema → Zod, re-registers tools/resources/prompts on the parent. URI ownership is tracked per mount; cross-upstream collisions fail fast at startup.
src/upstreams/index.ts Barrel + UpstreamHandle type.
src/index.ts Declares tools/resources/prompts capabilities, awaits mountUpstreams() before connecting stdio, binds child cleanup to SIGINT/SIGTERM/exit.
package.json Adds @ui5/webcomponents-react-mcp as a runtime dependency (spawned via require.resolve). No other runtime-dep changes.
.gitignore Adds .ab/ (umbrella for A/B-harness output, follow-up).

Tests

3 new serial tests in test/upstreams.test.ts:

  • ✅ Mounts a fixture upstream (test/fixtures/mock_upstream.mjs) and asserts tools, resources, and prompts are namespaced and forwarded with the correct prefix.
  • ✅ Mounts the real @ui5/webcomponents-react-mcp child and asserts the expected tools and the llms.txt resource appear.
  • ✅ Throws fast on cross-upstream resource-URI collisions.

Local run: 41/41 passing (38 existing + 3 new).

✔ upstreams › mountUpstream namespaces and forwards tools, resources, and prompts (106ms)
✔ upstreams › mountUpstream against real React MCP exposes expected tools and llms.txt resource (174ms)
✔ upstreams › mountUpstream throws on resource URI collision across upstreams (223ms)

Compatibility

  • No breaking changes to existing tool/resource/prompt names.
  • Forwarded items use a clear react_* prefix; native names stay as-is.
  • If the React child fails to spawn or list its capabilities, the parent fails fast with a clear error rather than starting in a half-mounted state.
  • Children are bound to the parent's lifecycle (SIGINT/SIGTERM/exit).

Caveats / known follow-ups

  • One upstream in the registry today. The whole point is the second is one line; we just haven't shipped it.
  • Hardcoded registry. No customer-supplied upstreams via config yet.
  • Stdio transport only. SSE/HTTP-transport upstreams not supported (npm-distributed servers covered today).
  • Names use a double prefix. The React MCP already namespaces its own tools, so you see react_react_get_component_api. Cosmetic, deterministic; can be cleaned up in a follow-up.

Manual verification

npm ci
npm run build
npm test          # 41/41 pass
./build/index.js  # boot manually; confirms mount log lines

…sses

Adds an opt-in mounting layer that lets this server spawn another MCP
server as a child over stdio and re-publish its tools, resources, and
prompts under a configurable name prefix. The first registered upstream
is @ui5/webcomponents-react-mcp, exposed under the 'react' prefix —
customers install one MCP and get framework-specific React guidance
through the same connection.

What this changes for users:
  - 'npm install @ui5/webcomponents-mcp-server' is enough to get the
    React MCP's tools as well; they appear as 'react_*'.
  - Native tools keep their existing names. No breaking changes to the
    public surface.
  - Adding a future framework MCP (Vue, Angular, …) is a one-entry
    addition to src/upstreams/registry.ts plus a runtime dependency.

Implementation:
  - src/upstreams/registry.ts — UPSTREAMS list (one entry today).
  - src/upstreams/mount.ts — spawns each upstream via the SDK's
    StdioClientTransport, lists its capabilities, converts JSON Schema
    inputs to Zod, and re-registers each item on the parent server.
    Resource URI ownership is tracked per mount so reads route to the
    right child. Cross-upstream URI collisions fail fast at startup.
  - src/upstreams/index.ts — barrel + UpstreamHandle type.
  - src/index.ts — declares tools/resources/prompts capabilities,
    awaits mountUpstreams() before connecting stdio, and binds child
    transport cleanup to SIGINT / SIGTERM / exit so children die with
    the parent.

Tests (test/upstreams.test.ts, serial):
  - mountUpstream namespaces and forwards tools, resources, and prompts
    against a fixture MCP server (test/fixtures/mock_upstream.mjs).
  - mountUpstream against the real @ui5/webcomponents-react-mcp child
    exposes the expected tools and llms.txt resource.
  - mountUpstream throws on resource URI collision across upstreams.

Dependencies:
  - Adds @ui5/webcomponents-react-mcp as a runtime dependency (it is
    spawned and resolved via require.resolve).
  - No other runtime dep changes.
Fixes raised in code review of feat/upstream-mounting:

  - Apply prettier formatting (2-space indent per .prettierrc) to
    mount.ts, registry.ts, mock_upstream.mjs. Was 4-space; would have
    produced noisy diffs on next contributor's auto-format.
  - mountUpstreams: detect duplicate prefixes in UPSTREAMS at startup
    and throw a clear error. Cheap insurance for when the registry
    grows past one entry.
  - resolveUpstreamBin: when bin is an object with multiple entries,
    prefer the one whose key matches the package basename (e.g.
    'webcomponents-react-mcp' for '@ui5/webcomponents-react-mcp').
    Falls back to the first entry, but no longer silently picks an
    iteration-order-dependent one when a future upstream ships
    multiple bins.
  - Resource handler: add a comment explaining the verbatim-forward
    trust model, matching tool/prompt forwarding.
  - promptArgsToZodShape: cite the MCP spec for prompt-args being
    string-only, to head off 'should this be typed?' churn.
  - shutdown(): comment why markShutdown() must come before
    transport.close() — close can fire onclose synchronously and
    would otherwise trip the upstream fail-fast handler on a planned
    exit.
  - _resetForTesting: doc-comment that callers must also build a
    fresh McpServer per mount, since this only resets module state.

Build clean, lint clean (no new errors), prettier --check clean,
41/41 tests pass including the 3 new upstream tests.
@dobrinyonkov

dobrinyonkov commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review - all the substantive fixes are in 9fb135a. Summary of what's addressed vs. deferred:

Addressed in this commit

Reviewer note Action
🔴 4-space indent breaks .prettierrc (tabWidth: 2) Re-formatted mount.ts, registry.ts, mock_upstream.mjs via prettier --write. prettier --check is now clean.
🟡 _resetForTesting doc Added doc-comment that callers must also build a fresh McpServer per mount.
🟡 Resource handler trust model One-line comment matching tool/prompt forwarding rationale.
🟡 No prefix-collision check Added seenPrefixes Set check at the top of mountUpstreams - throws on duplicate prefix in registry. (Test deferred - would require parameterizing mountUpstreams to take an upstreams list, which is API churn for a 3-line guard.)
🟡 Object.values(binField)[0] non-determinism Now prefers binField[basename] (e.g. webcomponents-react-mcp for @ui5/webcomponents-react-mcp); falls back to first entry only when no match.
🟢 Prompt-args spec citation Added one-line spec reference in promptArgsToZodShape.
🟢 markShutdown() ordering Added "Don't reorder" comment in shutdown() explaining the synchronous onclose race.

Pushed back on (with reasoning)

  • 🟡 Enum-with-one-value cast tightening - tried [ZodTypeAny, ZodTypeAny, ...] directly without the unknown step; TS rejects it because it can't statically prove literals has 2+ elements at the cast site. Kept the original double-cast and added a comment explaining why.

Deferred (follow-ups)

  • 🟢 .gitignore adding .ab/ - kept as part of this PR (acknowledged in commit/PR body); umbrella for the harness output dir, single line, low cost.
  • 🟢 README note about the React MCP runtime-dep size - separate docs PR.
  • 🟢 Tracking issue for react_react_* double-prefix - will open after merge with a concrete proposal (strip leading <prefix>_ from upstream tool names before mounting).
  • 🟢 Test for defaultOnUpstreamExit - agreed it'd be flaky.
  • 🟢 Test for richer schema converter (enum/number/array/default) - useful, follow-up; would expand the mock fixture rather than adding a unit test for the converter directly.

Build clean, lint clean (no new errors - the existing no-useless-escape in get_component_api.ts is unrelated and was already on main), 41/41 tests pass.

Correctness:

  - Drop process.on('exit', shutdown). The exit handler runs
    synchronously and ignores returned promises, so transport.close()
    couldn't reliably finish IPC teardown there. SIGINT/SIGTERM cover
    explicit teardown; child stdio servers receive SIGPIPE on next
    write after the parent exits, which is sufficient cleanup.
  - jsonValueSchemaToZod no longer applies .default(). The upstream
    is the single source of truth for validation and default-
    application — applying defaults twice could mask cases where
    undefined is meaningful to the upstream tool. Added a comment
    explaining why.
  - jsonObjectSchemaToZodShape: cleaner contract — "undefined"
    means "omit inputSchema; SDK skips parent-side validation",
    which is what we want for type:object schemas with no declared
    properties (free-form arg tools).
  - mountUpstream: symmetric child cleanup on connect-handshake
    failure, matching the post-connect path. No leaked children.
  - Upstream client connection now reports own version (read once
    from package.json), not '0.0.0'. Helpful in upstream debug logs.

API surface:

  - mountUpstreams now accepts an optional 'upstreams' parameter
    (defaults to UPSTREAMS) so the duplicate-prefix guard is
    testable without exposing module state.
  - _resetForTesting re-exported from the upstreams barrel, with a
    doc comment marking it test-only. Test imports moved to the
    barrel for consistency.

Documentation:

  - registerTool cast: clearer rationale (SDK overload depends on
    inputSchema presence; we choose dynamically based on upstream).
  - Tool name construction: comment near `${prefix}_${tool.name}`
    explaining the deliberate double-prefix policy and why we don't
    strip a leading match.

Tests (3 → 6, all serial):

  - Existing namespacing test now also exercises the richer
    schema converter (enum / integer / array / optional with default)
    via a second 'format' tool in the mock fixture.
  - NEW: mounted tool is callable through the parent server (uses
    InMemoryTransport.createLinkedPair to drive the parent end-to-end,
    not just the upstream client).
  - NEW: empty-capabilities upstream yields empty lists (covers the
    capability-gating short-circuit).
  - NEW: mountUpstreams throws on duplicate prefixes in the registry.
  - Added test/fixtures/mock_empty_upstream.mjs.
  - Both fixtures now handle SIGTERM/SIGINT cleanly so a hung test
    isn't left lying around.

Lint clean, prettier --check clean, 44/44 tests pass.
@dobrinyonkov

dobrinyonkov commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Round-2 fixes pushed in cfa1839. Per-item:

Correctness fixes (🟡 medium)

# Note Action
1 process.on('exit', shutdown) is sync; can't await transport.close() Removed. SIGINT/SIGTERM keep the explicit teardown path; stdio children get SIGPIPE on next write after parent exits, which is sufficient. Comment in index.ts explains why.
2 Defaults applied twice Fixed. Dropped .default() from jsonValueSchemaToZod; the upstream is now the single source of truth. Comment cites the rationale.
3 Empty-properties tool registers with no schema (rejects calls) Investigated and confirmed not a bug. The SDK skips parent-side validation entirely when inputSchema is undefined (see validateToolInput in mcp.js: if (!tool.inputSchema) return undefined;). So returning undefined for properties: {} correctly forwards calls verbatim. Cleaned up the converter's contract & comments to make this intentional. New empty-capabilities test covers the upstream-side angle.

Style / conventions (🟡)

# Note Action
4 Quote-style churn in index.ts The repo has .prettierrc with singleQuote: true already; the diff churn was prettier formatting the file on save. prettier --check is clean - single quotes is the project convention. Nothing to revert.
5 _resetForTesting imported directly, not via barrel Fixed. Re-exported from src/upstreams/index.ts with a "test-only" doc comment; test now imports from the barrel.
6 Cast comment unclear Improved. New comment explains the SDK overload depends on inputSchema presence, and we populate it dynamically.

Minor (🟢)

# Note Action
7 No client cleanup on connect() failure Fixed. Added symmetric transport.close() in the connect-failure path, matching the post-connect path.
8 Export defaultOnUpstreamExit for embedded consumers Deferred. No consumer asking for it yet; can export when one shows up. Tests already pass onUpstreamExit: () => {}.
9 Hardcoded version '0.0.0' for upstream client Fixed. New OWN_VERSION reads package.json once at module load (with safe '0.0.0' fallback).
10 Test fixture has no SIGTERM handler Fixed. Both fixtures (mock_upstream.mjs and the new empty one) now handle SIGTERM/SIGINT.
11 Module-global resourceOwners Deferred. Functional alternative is fine but is a bigger refactor; current approach with _resetForTesting() is honest and the API change cost is low when revisited.
12 Double-prefix not warned about Fixed. Added a code comment near ${prefix}_${tool.name} explaining the deliberate policy and why we don't strip a leading match.
13 SDK version bump callout The npm-shrinkwrap delta was indirect via the new dep; calling out explicitly: @modelcontextprotocol/sdk resolves at 1.27.1 for both this server and the React MCP - both were already on the same minor.

New tests (3 → 6)

  • Mounted tool is callable through the parent server - uses InMemoryTransport.createLinkedPair() to drive the full parent → registered handler → child path. Catches regressions in registerTool wiring that the existing client-direct test would miss.
  • Empty-capabilities upstream yields empty lists - verifies the capability-gating short-circuit (no tools/list against a server without tools capability).
  • mountUpstreams throws on duplicate prefixes in the registry - mountUpstreams now accepts an optional upstreams parameter so this is unit-testable without monkey-patching.
  • Existing namespacing test now also exercises the converter - second format tool in the mock fixture covers enum / integer / array / optional. The test additionally asserts that the upstream's default(false) applies (proving the parent did NOT also apply a default and short-circuit the upstream).

Verification

$ npm test
  …
  ✔ upstreams › mountUpstream namespaces and forwards tools, resources, and prompts (147ms)
  ✔ upstreams › mounted tool is callable through the parent server (125ms)
  ✔ upstreams › mountUpstream against an upstream with no capabilities yields empty lists (128ms)
  ✔ upstreams › mountUpstreams throws on duplicate prefixes in the registry
  ✔ upstreams › mountUpstream against real React MCP exposes expected tools and llms.txt resource (187ms)
  ✔ upstreams › mountUpstream throws on resource URI collision across upstreams (236ms)
  44 tests passed

prettier --check clean. No new lint errors (the existing no-useless-escape in get_component_api.ts is pre-existing).

Convention: ASCII hyphens only. Touches comments and string
literals; no behavioral change. Tests still 44/44.
@NakataCode
NakataCode self-requested a review June 24, 2026 11:01

@NakataCode NakataCode left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  1. onclose fires process.exit(1) during error-path cleanup, swallowing the error
    src/upstreams/mount.ts:159, 168–173

    After transport.onclose is set to defaultOnUpstreamExit (L159), the catch block calls
    await transport.close() (L168). The SDK fires the child process close event inside that
    await, which triggers onclose → process.exit(1). The throw error on L173 never reaches
    the caller.

  2. McpServer version hardcoded as '0.0.1' while package is 0.1.2
    src/index.ts:12

    The PR introduced OWN_VERSION/readOwnVersion() in mount.ts to fix the version reported
    to child upstreams, but the parent server's own identity advertised to MCP clients was not
    updated.

  3. No pagination for listTools / listResources / listPrompts
    src/upstreams/mount.ts:192, 231, 262

    All three SDK result types extend PaginatedResultSchema and include nextCursor. Only the
    first page is fetched. For the current React MCP (5 tools) this is invisible, but any upstream
    with more items than the default page size silently drops the rest with no error.

  4. Source comment incorrectly describes React MCP tool names
    src/upstreams/mount.ts:195–196

    Comment says "the React MCP exports tools like react_get_component_api" (pre-prefixed).
    Verified against the actual package v2.22.2: the React MCP exports get_component_api,
    create_app, etc. with no prefix. The test assertions are correct; the comment is wrong.

  5. Duplicate import of _resetForTesting
    test/upstreams.test.ts:9–10

    Two separate import statements from the same module specifier '../src/upstreams/index.js'.
    Should be merged into one.

- mount.ts: detach transport.onclose/onerror before awaiting transport.close()
  in both connect and registerUpstream catch blocks. The SDK's stdio transport
  fires onclose synchronously from the child's 'close' event inside close(),
  so defaultOnUpstreamExit would call process.exit(1) and swallow the thrown
  error before it could reach the caller.
- mount.ts: paginate listTools/listResources/listPrompts via new listAllPages
  helper. All three SDK list result types expose nextCursor; single-page reads
  silently drop items past the default page size.
- mount.ts: fix misleading naming comment - upstream tools are expected to be
  unprefixed (React MCP exports get_component_api, not react_get_component_api).
- index.ts: parent McpServer version now read from package.json, matching the
  pattern the PR already uses for the upstream-client identity. Was drifting
  behind (advertised '0.0.1' while package.json is at 0.1.2).
- test/upstreams.test.ts: merge duplicate _resetForTesting import.
- test/upstreams.test.ts: add regression test proving onUpstreamExit does NOT
  fire during error-path cleanup (verified to fail without the fix).
@NakataCode
NakataCode self-requested a review August 5, 2026 08:29
@NakataCode

Copy link
Copy Markdown

Found 2 issues:

1. mountUpstreams leaks already-spawned child processes on partial failure

src/upstreams/mount.ts lines 87–91: the loop accumulates handles with no cleanup path. If upstream N fails, all N-1 previously opened transports are never closed. Those child processes become orphans — StdioServerTransport has no stdin end/close handler so they don't self-exit on EOF, and SIGPIPE only fires on a write (an idle server waiting for requests never writes). process.exit(1) in main().catch masks this today, but it breaks down the moment a second upstream is added — which registry.ts explicitly plans for (Angular, Vue, ...).

const handles: UpstreamHandle[] = [];
for (const spec of upstreams) {
  handles.push(await mountUpstream(server, spec)); // no cleanup of handles[0..N-2] if this throws
}

2. handles is [] during the entire startup mount window

src/index.ts lines 42 and 75: handles is only assigned after mountUpstreams fully resolves, but the child process is spawned inside client.connect() early in mountUpstream. There is a ~100ms gap (consistent with test timings) where the child is alive but handles is still []. A SIGINT in that window causes shutdown() to iterate an empty array and close nothing — the child becomes an orphan for the same reason as above. This affects the current single-upstream setup, not just a future multi-upstream scenario.

let handles: UpstreamHandle[] = [];         // line 42 — initialized empty
// child is alive from ~1ms into mountUpstream...
handles = await mountUpstreams(server);      // line 75 — assigned ~100ms later

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.

[Feature Request] Adding @ui5/webcomponents-react

2 participants