MCP server: post-merge follow-ups from PR #148 review

[enko]

Follow-up cleanup items from the PR #148 (feat(all): Add MCP server for LLM data access) code review. None block merge; capture here so they don't get lost.

Medium

1. docker/nginx.dev.conf missing mcp_auth rate-limit zone

docker/nginx.prod.conf, docker/nginx.conf.template, docker/nginx.multiservice.conf.template all declare:

limit_req_zone $binary_remote_addr zone=mcp_auth:10m rate=30r/m;

docker/nginx.dev.conf does not. Today the dev compose stack does not expose /mcp with limit_req, so this is latent — but if anyone copies a limit_req zone=mcp_auth ... directive into the dev conf later, nginx config parse will fail.

Fix: add the zone to nginx.dev.conf for parity, or add a comment explicitly stating dev intentionally omits rate-limiting.

2. No idle-session TTL / pruning in MCP server

apps/mcp-server/src/index.ts:19 keeps sessions: Map<string, Session> whose entries are only deleted on:

• explicit DELETE /mcp from a client (http-handler.ts:285-294), or
• transport onclose (http-handler.ts:253-256).

Long-lived clients that vanish without closing (network partition, force-killed Claude Desktop) leak entries indefinitely. Memory grows unbounded over a process's lifetime.

Fix: track lastSeenAt on each session, run a periodic sweep (e.g. every 5 min) that calls transport.close() on sessions idle for > N minutes (e.g. 60 min), and remove from the map. Add a corresponding integration test.

Low

3. http-handler.ts:292 — needless as string cast

sessions.delete(sessionId as string);

sessionId is typed string | undefined from the outer scope. TS doesn't narrow it through the if (deleteSession) block even though deleteSession was looked up via sessions.get(sessionId).

Fix: introduce a local narrowed binding, e.g. if (sessionId && (deleteSession = sessions.get(sessionId))), or assign a const id = sessionId! after the deleteSession check.

4. scripts/sync-versions.mjs accepts any string

The script writes process.argv[2] to every package.json without validating semver shape. Low risk because semantic-release controls the call, but a stray manual invocation with a typo silently rewrites every workspace package.

Fix: validate against semver regex (or the semver package) before writing; exit non-zero on bad input.

5. No tool-parameter validation tests

apps/mcp-server/src/tools/{friends,circles,collectives,encounters}.ts use zod schemas to validate inputs. No test covers the failure paths — invalid UUID, out-of-range page size, malformed date, etc. Today the tools rely on PgTyped to reject malformed input downstream; an explicit assertion at the schema layer would catch regressions if a schema is loosened or removed.

Fix: add a test per tool registration that calls the tool with a deliberately bad argument and asserts a structured zod error response.

6. No malformed JSON-RPC body test

http-handler.ts:222-224 parses the body as JSON then hands the result to transport.handleRequest without shape validation at the handler layer. The MCP SDK enforces the JSON-RPC envelope, but a failing-fast test (e.g. { "not": "a-jsonrpc-message" }) would document the expectation and catch regressions if the SDK behaviour changes.

Fix: integration test that POSTs a non-JSON-RPC body to /mcp and asserts a 4xx response.

7. getClientIp test mock typing

apps/mcp-server/tests/get-client-ip.test.ts:7-14 casts the mock via as unknown as IncomingMessage. Acceptable for a unit test, but a typed helper (Partial<IncomingMessage> plus a small builder) would be cleaner and easier to extend.

Fix: introduce a makeMockIncomingMessage helper in a shared test util once a second test file needs the same mock.

---

Source: code review on PR #148. All issues verified against the merged branch.