Replace monolithic MCP server with a layered proxy - #45
Open
smartlabsAT wants to merge 1 commit into
Open
Conversation
Collapse 2223 LOC of inline tool registrations, browser lifecycle, and
state mixing in `src/mcp/server.ts` into a thin routing layer over an
upstream MCP execution layer. Mechanical refactor — no new tool logic.
New `src/proxy/` module:
- `proxy-server.ts` — MCP `Server` (low-level), `setRequestHandler` for
`ListToolsRequestSchema` + `CallToolRequestSchema`, an inline (currently
empty) local-tool registry, and SIGINT/SIGTERM cleanup.
- `upstream-client.ts` — wraps `@playwright/mcp@0.0.75` as a subprocess
via `Client` + `StdioClientTransport`. Lazy single-flight start,
auto-restart on `transport.onclose` with a 5-per-60s rate-limit
exiting on overrun, and `restartWith(options)` exposed for Epic 3.
- `version.ts` — reads PACKAGE_NAME / PACKAGE_VERSION from package.json
so MCP handshake metadata follows version bumps automatically.
The `@playwright/mcp` CLI entry is resolved via
`require.resolve('@playwright/mcp/package.json')` rather than `/cli.js`
because v0.0.75 has a strict `exports` field that does not expose
`./cli.js` — we derive the sibling path. `PLAYWRIGHT_MCP_CLI_PATH` is
honoured as an override and both code paths are guarded with helpful
errors when the package or file is missing.
`src/mcp/server.ts` becomes a 3-line shim that imports the proxy, so
existing `.mcp.json` configurations pointing at `dist/mcp/server.cjs`
continue to work without modification.
Protocol-validation modules (`protocol-validator.ts`,
`protocol-validation-layer.ts`, `protocol-error-recovery.ts`,
~933 LOC) are deleted along with their three tests; SDK-level protocol
handling now covers what they did. Their four public re-exports are
removed from `src/index.ts`.
`tsup.config.ts` gains the proxy entry and adds `@playwright/mcp` to
externals across the MCP and proxy bundles. `package.json` pins
`@playwright/mcp` at exact `0.0.75`, bumps `engines.node` from `>=16`
to `>=18`, and the bin/exports entries stay valid because the shim
still produces `dist/mcp/server.cjs`.
Out-of-scope per the issue: no cache/click/type interception (Epic 2),
no local session/test/cache tools (Epic 3 will populate the registry),
no `--caps` flag forwarding, no BASE_URL URL rewriting (acknowledged
alpha.1 regression — Epic 2 restores it).
Verified behaviour:
- `npx tsc` and `npm run build` exit 0; `npm test` 15/15 passes.
- A spawn-and-list smoke test connects in 71 ms and lists 23 upstream
tools (well under the 3 s AC); a real `browser_navigate` round-trip
completes in ~1.1 s including browser launch.
- Sequential load (50 calls): ~44 ms/call after warm-up, single stable
upstream PID, no memory growth.
- Concurrent `tools/list` (10x): single-flight start verified — upstream
spawns exactly once.
- Crash + auto-restart recovers in ~2.2 s; six successive crashes hit
the rate limit and the proxy exits cleanly with code 1.
- SIGINT/SIGTERM: exit 0 in ~3 ms, no zombie subprocesses.
- Backward-compat: `dist/mcp/server.cjs` shim and `dist/proxy/proxy-server.cjs`
direct entry are functionally identical (6-byte size delta from the
source-map URL only).
Review fixes applied before commit:
- `tools/list` errors now propagate to the client (was silently returning
an empty list, indistinguishable from "no tools available").
- Missing `@playwright/mcp` or invalid `PLAYWRIGHT_MCP_CLI_PATH` now
produce actionable error messages instead of Node's `MODULE_NOT_FOUND`
stack trace.
- `stop()` clears the pending restart timer and detaches the old
transport's `onclose` to prevent a stale handler firing into a fresh
client instance.
- `restartWith()` carries an inline TODO documenting the Epic-3 race
window between `stop()` and `ensureStarted()`.
Docs (`docs/MCP_SERVER.md`, `docs/INTELLIGENT_TESTING.md`,
`docs/CACHING.md`, `docs/ISSUE_24_STABILITY.md`) get explicit
"v0.2.0-alpha.1 status" notices so users reading them understand that
session/test/cache MCP tools are temporarily unavailable in alpha.1
and return in a later v0.2.0 milestone (CLI equivalents work unchanged).
Closes #39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the 2223-LOC monolithic
src/mcp/server.tswith a thin routing layer over an upstream MCP execution layer. Pure architectural refactor — no new tool logic. Tool delegation, browser lifecycle, and signal handling all move out of the monolith into a small focused module.New
src/proxy/moduleproxy-server.tsServer(low-level),setRequestHandlerforListToolsRequestSchema+CallToolRequestSchema, inline local-tool registry, signal cleanupupstream-client.ts@playwright/mcp@0.0.75as a subprocess viaClient+StdioClientTransport. Single-flight start, auto-restart with rate limit,restartWith(options)exposed for Epic 3version.tsPACKAGE_NAME/PACKAGE_VERSIONfrompackage.jsonfor MCP handshake metadataOther changes
src/mcp/server.tsbecomes a 3-line shim importing the proxy — existing.mcp.jsonconfigs pointing atdist/mcp/server.cjskeep working@playwright/mcp@0.0.75added as exact-pinned dependencyengines.nodebumped>=16→>=18tsup.config.tsgains the proxy entry;@playwright/mcpbecomes external in both MCP and proxy bundlesprotocol-validator.ts,protocol-validation-layer.ts,protocol-error-recovery.tssrc/index.tsMCP_SERVER.md,INTELLIGENT_TESTING.md,CACHING.md,ISSUE_24_STABILITY.md) get a "v0.2.0-alpha.1 status" banner so users understand session/test/cache MCP tools are temporarily unavailablesrc/commands/mcp.tsCLI hint refreshed to list real upstream tools instead of removed onesStats
18 files changed: +436 / −3807 (netto −3371 LOC in src/tests).
Architecture decisions honoured (Epic #35)
Serverlow-level class — notMcpServer@playwright/mcp@0.0.75exact pin (no caret)require.resolveresolution — actually via@playwright/mcp/package.jsonbecause v0.0.75 has a strictexportsfield that does not expose./cli.js; we derive the sibling pathsrc/mcp/server.tsbrowser_*prefix preserved (all 23 upstream tools)Out of scope (per issue, intentionally absent)
--capsflag forwardingBASE_URLenv-var URL rewriting (acknowledged alpha.1 regression, restored in Epic 2)Review fixes applied before commit
After dispatching two parallel review agents (code-quality + stress-test), I applied four fixes:
tools/listpropagates upstream errors instead of silently returning an empty list (MCP error -32603 surfaces the real diagnosis to the client)@playwright/mcpand invalidPLAYWRIGHT_MCP_CLI_PATHinstead of Node'sMODULE_NOT_FOUNDstack tracestop()cleanup clears the pending restart timer and detaches the old transport'sonclose— prevents a stale callback firing into a fresh clientrestartWith()documenting the Epic-3 race window betweenstop()andensureStarted()Closes #39
Test plan
npm run build:tscexits 0npm run buildexits 0npm testexits 0 (15/15: 8 CLI + 7 install)dist/mcp/server.cjsanddist/proxy/proxy-server.cjsexist and are executable; both list 23 toolsdist/mcp/server.cjs, connect,tools/listreturns ≥ 23 tools in under 3 s (target was 3 s, observed 71 ms)browser_navigatetoabout:blankfollowed bybrowser_closecompletes successfully (verified end-to-end)tools/listcalls trigger upstream startup exactly once (single-flight)kill -9on the upstream subprocess; next call recovers in ~2 sisError: truefrom upstream, proxy stays alivePLAYWRIGHT_MCP_CLI_PATHsurfaces the actionable error message