From 6b54c590bad7a6fe0ca58a158471e293904b0a9e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 05:08:41 +0000 Subject: [PATCH 1/2] fix(runtime,rest): advertise `mcp` only when it is actually serveable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #4024. Both discovery producers gated the `/mcp` route on `isMcpServerEnabled()` alone. The dispatcher documented why that was safe: `os serve` auto-loads plugin-mcp from the same flag, so on that path advertised did imply mounted. That reasoning is sound but it describes the CLI, not the dispatcher — and the same comment claimed `@objectstack/rest` stayed "symmetric", which it did not. `@objectstack/rest` has no `@objectstack/mcp` dependency, mounts no `/mcp` route and performs no auto-load, so a host embedding it without plugin-mcp advertised `/mcp` in `/discovery` and then answered 501 on it. Every OTHER optional route in that discovery table is service-presence gated (hasUi, hasFiles, hasAnalytics, hasI18n, …). `mcp` was the lone exception, and the exception is what broke. That matters beyond tidiness: `/discovery` is the contract a third-party client reads to decide what exists, and the objectui Integrations page renders its MCP connect card from this field. Advertising an endpoint that 501s makes discovery unreliable for exactly the consumers that depend on it. Now: flag AND serveable, on both producers. - Dispatcher gates on the handler's OWN predicate — `typeof mcpSvc?.handleHttpRequest === 'function'`, byte-identical to what handleMcpRequest 501s on. Gating on mere service presence would still over-promise when a wrong-shaped service is registered. - `@objectstack/rest` probes via the per-request kernel (shape) or the single-env `serviceExistsProvider` (existence) — the same two paths `resolveRegisteredServices` already uses. `rest-api-plugin` always wires the latter, so the gate is live in real deployments rather than decorative. When it genuinely cannot probe it keeps the prior flag-only answer: "unknown" is not "absent", and hiding a working endpoint would be the opposite over-correction (fail-open, ADR-0057 D10). `os serve` / `os dev` are unaffected — they load the plugin, so the service resolves and `/mcp` is still advertised. ## Reproduced, then fixed #4024 was filed as static analysis with the repro explicitly listed as not done. It reproduces. With the flag on and the service absent, the old gate failed the new pairing assertion with the defect stated verbatim: AssertionError: discovery advertised mcp (true) while POST /mcp returned 501 — declared ≠ enforced ## The seam that should have caught it `route-parity.integration.test.ts` has carried an `mcp` opt-out in `stubServicesPlugin` since it was written, and nothing ever passed it — `bootServe()` was called with no args and with `{ notification: false }`, never `{ mcp: false }`. Its MCP test ("MCP is advertised AND reachable") stubs the service in unconditionally, proving the lockstep only under the one condition where it cannot fail. So the single capability whose advertisement was not service-presence gated was also the single one whose absence was never tested. The seam is now used. Also added rest-side coverage for absent / present / unprobeable, since the three existing `routes.mcp` tests construct RestServer with three args and therefore all take the unprobeable path — without the new cases the rest half would have shipped with no coverage at all. Verified each new assertion fails against the old gate and passes against the new one. runtime + rest: 906 passed (65 files); rest alone 494 (34 files); repo eslint clean; 0 type errors. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011AvZj6cLX7APd7roh2eK4F --- .changeset/mcp-discovery-service-aware.md | 30 ++++++++ packages/rest/src/rest-server.ts | 53 +++++++++++++- packages/rest/src/rest.test.ts | 56 ++++++++++++++- packages/runtime/src/http-dispatcher.ts | 39 ++++++---- .../src/route-parity.integration.test.ts | 72 ++++++++++++++++++- 5 files changed, 228 insertions(+), 22 deletions(-) create mode 100644 .changeset/mcp-discovery-service-aware.md diff --git a/.changeset/mcp-discovery-service-aware.md b/.changeset/mcp-discovery-service-aware.md new file mode 100644 index 0000000000..fffef5bba1 --- /dev/null +++ b/.changeset/mcp-discovery-service-aware.md @@ -0,0 +1,30 @@ +--- +"@objectstack/runtime": patch +"@objectstack/rest": patch +--- + +Advertise `mcp` in `/discovery` only when it is actually serveable (#4024). + +Both discovery producers gated the `/mcp` route on `isMcpServerEnabled()` alone. +The stated justification was a lockstep — `os serve` auto-loads plugin-mcp from +the same flag, so on that path advertised did imply mounted. But the lockstep is +a property of the CLI, not of the dispatcher: `@objectstack/rest` has no +`@objectstack/mcp` dependency, mounts no `/mcp` route and performs no auto-load, +so a host that embedded it without plugin-mcp advertised `/mcp` in `/discovery` +and then answered 501 on it — the `declared ≠ enforced` failure #3369 forbids, +and a broken contract for third-party clients that read `/discovery` to decide +what exists. + +Both producers now require the flag AND a serveable MCP service. The runtime +dispatcher gates on the handler's own predicate (`typeof +mcp.handleHttpRequest === 'function'`), so a wrong-shaped service can't +over-promise either. `@objectstack/rest` probes via the per-request kernel or the +single-env `serviceExistsProvider`; when it genuinely cannot probe it keeps the +prior flag-only answer rather than hiding a working endpoint (fail-open, +ADR-0057 D10). The `os serve` / `os dev` path is unchanged — it loads the plugin, +so the service resolves and `/mcp` is still advertised. + +Also exercises the `mcp: false` seam in `route-parity.integration.test.ts`, which +had existed unused since the file was written: `bootServe()` was only ever called +with no args or `{ notification: false }`. The one capability whose advertisement +was not service-presence gated was also the one whose absence was never tested. diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 033f96cbc9..9ed935bc6b 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -2127,6 +2127,41 @@ export class RestServer { } } + /** + * Is `/mcp` actually serveable — i.e. is the MCP service registered with + * the shape `handleMcpRequest` needs? + * + * `true`/`false` are answers; `null` means "could not probe". The route + * itself is served by the runtime dispatcher (`domains/mcp.ts`), which + * 501s on `!mcp || typeof mcp.handleHttpRequest !== 'function'` — so this + * probe exists to keep our `/discovery` from advertising a route that + * would 501 (#4024). + * + * Same two probe paths as {@link resolveRegisteredServices} (ADR-0057 + * D10): the per-request kernel for multi-env hosts, else the single-env + * `serviceExistsProvider` — which `rest-api-plugin` always wires. Via the + * kernel we can check the SHAPE; the single-env provider answers existence + * only, which is the dominant case (the dispatcher's own service-aware + * discovery covers the wrong-shape case). + */ + private async probeMcpServeable(req: any): Promise { + try { + let environmentId: string | undefined = req?.params?.environmentId; + if ((!environmentId || environmentId === ':environmentId') && this.defaultEnvironmentIdProvider) { + try { environmentId = this.defaultEnvironmentIdProvider() || undefined; } catch { /* ignore */ } + } + if (environmentId && environmentId !== 'platform' && this.kernelManager) { + const kernel: any = await this.kernelManager.getOrCreate(environmentId); + if (kernel && typeof kernel.getServiceAsync === 'function') { + const svc: any = await kernel.getServiceAsync('mcp').catch(() => undefined); + return typeof svc?.handleHttpRequest === 'function'; + } + } + if (this.serviceExistsProvider) return this.serviceExistsProvider('mcp') === true; + } catch { /* fall through to "cannot probe" */ } + return null; + } + /** * Register discovery endpoints */ @@ -2166,8 +2201,22 @@ export class RestServer { // project-scoped), so point at the unscoped base. This // `/discovery` (served by @objectstack/rest) is separate // from the dispatcher's getDiscoveryInfo — both must - // advertise `mcp` (single source: isMcpServerEnabled). - if (isMcpServerEnabled()) { + // advertise `mcp` on the same terms. + // + // Enabled is NOT the same as serveable (#4024). The flag + // alone used to gate this, on the reasoning that `os serve` + // auto-loads plugin-mcp from the same flag. But that + // lockstep belongs to the CLI: `@objectstack/rest` has no + // `@objectstack/mcp` dependency, mounts no /mcp route and + // performs no auto-load, so an embedder that skips + // plugin-mcp had `mcp` advertised here while the route + // 501'd — the `declared ≠ enforced` failure #3369 forbids. + // A `null` probe means we genuinely cannot tell; keep the + // old flag-only answer there rather than hiding a working + // endpoint (fail-open, ADR-0057 D10) — the dispatcher's own + // discovery is service-aware and stays authoritative. + const mcpServeable = await this.probeMcpServeable(req); + if (isMcpServerEnabled() && mcpServeable !== false) { const unscopedBase = isScoped ? basePath.replace(/\/(environments|projects)\/:environmentId$/, '') : basePath; diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index 84f7713a1a..a3d377dde3 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -2423,12 +2423,33 @@ describe('mapDataError — schema/constraint envelopes', () => { // --------------------------------------------------------------------------- describe('discovery — routes.mcp (ADR-0036, #152)', () => { - function discoveryHandler() { + /** + * `serviceExists` omitted → the server cannot probe whether MCP is actually + * serveable, so it keeps the flag-only answer (fail-open, ADR-0057 D10). + * Pass one to exercise the service-aware gate added for #4024. + */ + function discoveryHandler(serviceExists?: (name: string) => boolean) { const server = createMockServer(); const protocol = createMockProtocol(); // protocol discovery carries a `routes` object the server augments. (protocol.getDiscovery as any) = vi.fn().mockResolvedValue({ routes: { data: '', metadata: '' } }); - const rest = new RestServer(server as any, protocol as any, ANON_API as any); + const rest = new RestServer( + server as any, protocol as any, ANON_API as any, + undefined, // kernelManager + undefined, // envRegistry + undefined, // defaultEnvironmentIdProvider + undefined, // authServiceProvider + undefined, // objectQLProvider + undefined, // emailServiceProvider + undefined, // sharingServiceProvider + undefined, // reportsServiceProvider + undefined, // approvalsServiceProvider + undefined, // sharingRulesServiceProvider + undefined, // i18nServiceProvider + undefined, // analyticsServiceProvider + undefined, // settingsServiceProvider + serviceExists, + ); rest.registerRoutes(); const entry = rest.getRouteManager().get('GET', '/api/v1/discovery'); if (!entry) throw new Error('discovery route not registered'); @@ -2465,6 +2486,37 @@ describe('discovery — routes.mcp (ADR-0036, #152)', () => { const body = await invoke(discoveryHandler()); expect(body.routes.mcp).toBeUndefined(); }); + + // ── #4024: enabled ≠ serveable ────────────────────────────────────────── + // The flag alone used to decide this. But @objectstack/rest has no + // @objectstack/mcp dependency, mounts no /mcp route and performs no + // auto-load — that lockstep belongs to `os serve`. So an embedder without + // plugin-mcp advertised /mcp here and got 501 from the runtime dispatcher, + // which is the `declared ≠ enforced` failure #3369 forbids. + + it('omits routes.mcp when enabled but the mcp service is absent (#4024)', async () => { + delete process.env.OS_MCP_SERVER_ENABLED; // default-on + const body = await invoke(discoveryHandler((name) => name !== 'mcp')); + expect( + body.routes.mcp, + 'enabled-but-unserveable must not be advertised — it would 501', + ).toBeUndefined(); + }); + + it('advertises routes.mcp when enabled AND the mcp service is present (#4024)', async () => { + delete process.env.OS_MCP_SERVER_ENABLED; + const body = await invoke(discoveryHandler(() => true)); + expect(body.routes.mcp).toBe('/api/v1/mcp'); + }); + + it('keeps the flag-only answer when the host cannot be probed (fail-open, ADR-0057 D10)', async () => { + // No kernelManager and no serviceExistsProvider → "unknown", not "absent". + // Hiding a working endpoint here would be the opposite over-correction, and + // the dispatcher's own service-aware discovery stays authoritative. + delete process.env.OS_MCP_SERVER_ENABLED; + const body = await invoke(discoveryHandler()); + expect(body.routes.mcp).toBe('/api/v1/mcp'); + }); }); // ────────────────────────────────────────────────────────────────────────── diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index c2c73d9294..5353e6f32c 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -856,7 +856,7 @@ export class HttpDispatcher { const [ authSvc, searchSvc, realtimeSvc, filesSvc, analyticsSvc, workflowSvc, aiSvc, notificationSvc, i18nSvc, - uiSvc, automationSvc, cacheSvc, queueSvc, jobSvc, + uiSvc, automationSvc, cacheSvc, queueSvc, jobSvc, mcpSvc, ] = await Promise.all([ this.resolveService(CoreServiceName.enum.auth), this.resolveService(CoreServiceName.enum.search), @@ -872,6 +872,9 @@ export class HttpDispatcher { this.resolveService(CoreServiceName.enum.cache), this.resolveService(CoreServiceName.enum.queue), this.resolveService(CoreServiceName.enum.job), + // Not a CoreServiceName — plugin-mcp registers under the bare + // 'mcp' key, the same string handleMcpRequest resolves. + this.resolveService('mcp'), ]); const hasAuth = !!authSvc; @@ -887,6 +890,11 @@ export class HttpDispatcher { const hasCache = !!cacheSvc; const hasQueue = !!queueSvc; const hasJob = !!jobSvc; + // Mirrors handleMcpRequest's OWN guard byte for byte (domains/mcp.ts): + // it 501s on `!mcp || typeof mcp.handleHttpRequest !== 'function'`, so + // advertising on mere service presence would still over-promise when a + // wrong-shaped service is registered. Same predicate ⇒ same answer. + const hasMcp = typeof mcpSvc?.handleHttpRequest === 'function'; // Routes are only exposed when a plugin provides the service const routes = { @@ -908,21 +916,22 @@ export class HttpDispatcher { notifications: hasNotification ? `${prefix}/notifications` : undefined, ai: hasAi ? `${prefix}/ai` : undefined, i18n: hasI18n ? `${prefix}/i18n` : undefined, - // MCP (Streamable HTTP) is a default-on core capability — - // advertised unless OS_MCP_SERVER_ENABLED=false opts the env - // out. The objectui Integrations page reads this. + // MCP (Streamable HTTP) is a default-on core capability — but + // "enabled" and "serveable" are two different facts and both + // must hold. The objectui Integrations page reads this. // - // `declared === enforced` here is guaranteed by a LOCKSTEP, not - // by service-presence gating like the routes above (#3369 / - // #2698): `os serve` auto-loads plugin-mcp from the SAME - // `isMcpServerEnabled()` flag that gates this advertisement, so - // whenever `/mcp` is advertised the handler is mounted (a key / - // token yields 401, never a 404/501). Kept flag-based on purpose - // — `@objectstack/rest` advertises `mcp` from the identical - // single source (rest-server.ts), so the two discovery producers - // stay symmetric. The route-parity gate asserts the lockstep - // holds (advertised ⇒ reachable, never 501). - mcp: isMcpServerEnabled() ? `${prefix}/mcp` : undefined, + // Service-presence gated like every other optional route above + // (#3369 / #2698), NOT flag-only (#4024). This used to trust a + // LOCKSTEP instead: `os serve` auto-loads plugin-mcp from the + // same `isMcpServerEnabled()` flag, so on that path advertised + // did imply mounted. But the lockstep is a property of the CLI, + // not of the dispatcher — an embedder that mounts the dispatcher + // (or `@objectstack/rest`) without plugin-mcp got `/mcp` + // advertised here and 501 from handleMcpRequest, which is + // exactly the `declared ≠ enforced` failure #3369 forbids. + // Gating on the handler's own predicate makes the invariant hold + // by construction on every host instead of by convention on one. + mcp: isMcpServerEnabled() && hasMcp ? `${prefix}/mcp` : undefined, }; // Build per-service status map diff --git a/packages/runtime/src/route-parity.integration.test.ts b/packages/runtime/src/route-parity.integration.test.ts index ad42d6244a..243ee65b27 100644 --- a/packages/runtime/src/route-parity.integration.test.ts +++ b/packages/runtime/src/route-parity.integration.test.ts @@ -3,9 +3,17 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { LiteKernel } from '@objectstack/core'; import { HonoServerPlugin } from '@objectstack/plugin-hono-server'; +import { isMcpServerEnabled } from '@objectstack/types'; import { createDispatcherPlugin } from './dispatcher-plugin.js'; +/** + * The statuses that mean "advertised but not actually served" — 404 (route not + * mounted), 405 (wrong-method sink), 501 (no backing handler/service). Shared + * by the probe loop and the #4024 MCP pairing check. + */ +const DEAD_MCP_STATUSES = new Set([404, 405, 501]); + /** * Route-parity gate — `declared === enforced` for HTTP routes (issue #3369). * @@ -156,7 +164,6 @@ describe('Route parity: discovery-advertised routes are reachable on os serve (# * declared ≠ enforced. An anonymous caller legitimately gets 401/403; that * still proves the route is mounted. */ - const DEAD_STATUSES = new Set([404, 405, 501]); const probes: Array<{ method: string; path: string; note: string }> = [ { method: 'GET', path: '/api/v1/health', note: 'liveness probe' }, { method: 'GET', path: '/api/v1/ready', note: 'readiness probe' }, @@ -170,7 +177,7 @@ describe('Route parity: discovery-advertised routes are reachable on os serve (# for (const { method, path, note } of probes) { const res = await fetch(`${baseUrl}${path}`, { method }); expect( - DEAD_STATUSES.has(res.status), + DEAD_MCP_STATUSES.has(res.status), `${method} ${path} (${note}) returned ${res.status} — declared but not enforced`, ).toBe(false); } @@ -180,7 +187,7 @@ describe('Route parity: discovery-advertised routes are reachable on os serve (# for (const { method, path, note } of probes) { const res = await fetch(`${baseUrl}${path}`, { method, headers: { 'x-test-user': 'admin1' } }); expect( - DEAD_STATUSES.has(res.status), + DEAD_MCP_STATUSES.has(res.status), `${method} ${path} (${note}) returned ${res.status} for admin — declared but not enforced`, ).toBe(false); } @@ -236,3 +243,62 @@ describe('Route parity: discovery is service-aware — no dead advertisement (#3 expect(res.status).toBe(404); }); }); + +/** + * The MCP half of the same invariant (#4024). + * + * `stubServicesPlugin` has carried an `mcp` opt-out since it was written, and + * NOTHING ever passed it: `bootServe()` was called with no args and with + * `{ notification: false }`, never `{ mcp: false }`. So the one capability whose + * advertisement was NOT service-presence gated was also the one whose absence + * was never tested — the sibling test above ("MCP is advertised AND reachable") + * stubs the service in unconditionally, proving the lockstep only under the + * condition where it cannot fail. + * + * That gap hid a real exposure. `/mcp` was advertised on `isMcpServerEnabled()` + * alone, justified by `os serve` auto-loading plugin-mcp from the same flag — + * but that lockstep is a property of the CLI, not of the dispatcher. Any host + * that mounts the dispatcher (or `@objectstack/rest`, which has no + * `@objectstack/mcp` dependency and no auto-load) without plugin-mcp advertised + * `/mcp` and then 501'd on it. + * + * With the flag ON and the service ABSENT, the honest answer is: don't + * advertise it. Note the flag stays untouched here — this is specifically the + * enabled-but-unserveable case, not the opted-out one. + */ +describe('Route parity: MCP is service-aware — enabled but unserveable is not advertised (#4024)', () => { + let kernel: LiteKernel; + let baseUrl: string; + + beforeAll(async () => { + // Boot WITHOUT the mcp service, flag left at its default-on value. + ({ kernel, baseUrl } = await bootServe({ mcp: false })); + }, 30_000); + + afterAll(async () => { if (kernel) await shutdown(kernel); }, 30_000); + + it('does NOT advertise mcp when the service is absent, even though the flag is on', async () => { + expect(isMcpServerEnabled(), 'precondition: the default-on flag must still be on').toBe(true); + const disc = await (await fetch(`${baseUrl}/api/v1/discovery`)).json(); + expect( + disc.data.routes.mcp, + 'mcp must NOT be advertised when no mcp service is registered (declared === enforced)', + ).toBeFalsy(); + }); + + it('never advertises mcp while the route 501s — the exact declared-vs-enforced pair', async () => { + // The pairing IS the invariant, so assert it as a pair rather than + // pinning the un-provisioned status on its own: whatever /mcp answers, + // discovery must not be promising it. This is the assertion that would + // have caught #4024, and it holds under either resolution (stop + // advertising, or start serving). + const disc = await (await fetch(`${baseUrl}/api/v1/discovery`)).json(); + const res = await fetch(`${baseUrl}/api/v1/mcp`, { method: 'POST', body: '{}' }); + const advertised = Boolean(disc.data.routes.mcp); + const dead = DEAD_MCP_STATUSES.has(res.status); + expect( + advertised && dead, + `discovery advertised mcp (${advertised}) while POST /mcp returned ${res.status} — declared ≠ enforced`, + ).toBe(false); + }); +}); From fdde8e03222903e2793bd181ed57dbb96a55d027 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 05:10:56 +0000 Subject: [PATCH 2/2] docs(ai): record the enabled-but-unserveable MCP symptom MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The connect-MCP troubleshooting table had a `501 Not Implemented` row ("the MCP plugin isn't part of this build"). That row is still accurate — the route stays mounted and still 501s when called directly; only the advertisement changed. But it is now incomplete: after the #4024 fix the first signal an integrator sees is usually the *absence* of `mcp` from `/discovery` and a missing Connect-an-Agent card, not a 501, because discovery no longer points at a route it cannot serve. Added the other-side-of-the-same-cause row, including the distinction that matters for embedders: `os serve` / `os dev` load the plugin for you, while a host embedding `@objectstack/rest` directly must add `@objectstack/mcp` itself. Note: the docs-drift advisory on #4032 listed 22 files derived from the dependency graph and did NOT include this one — the doc that actually needed the change. Recording that here rather than filing it; the advisory is explicitly best-effort. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011AvZj6cLX7APd7roh2eK4F --- content/docs/ai/connect-mcp.mdx | 1 + 1 file changed, 1 insertion(+) diff --git a/content/docs/ai/connect-mcp.mdx b/content/docs/ai/connect-mcp.mdx index 5e8c95baa5..c6cf8acecb 100644 --- a/content/docs/ai/connect-mcp.mdx +++ b/content/docs/ai/connect-mcp.mdx @@ -191,6 +191,7 @@ skill and a guided `/objectstack:connect` command. |:---|:---| | `404` on `/api/v1/mcp` | The HTTP surface is disabled — unset `OS_MCP_SERVER_ENABLED` (default is on) | | `501 Not Implemented` | The MCP plugin isn't part of this build — check your stack's plugins | +| `mcp` missing from `GET /api/v1/discovery` and no Connect-an-Agent card, but `OS_MCP_SERVER_ENABLED` is on | The same cause as the `501` above, seen from the other side: the surface is *enabled* but not *serveable*, so discovery declines to advertise a route that would 501 rather than over-promising it (`declared === enforced`). Load the MCP plugin — `os serve` / `os dev` do it for you; a host that embeds `@objectstack/rest` directly must add `@objectstack/mcp` itself | | stdio won't start / boot fails closed | `OS_MCP_STDIO_ENABLED=true` but `OS_MCP_STDIO_API_KEY` is missing, unknown, revoked, or expired — fail-closed by design (ADR-0101). Set a valid `osk_` key; there is no unscoped or `system` fallback | | `401` on every call | Anonymous or invalid credentials. Interactive clients: complete the browser login (the `WWW-Authenticate` header advertises the OAuth metadata). Headless: check the `osk_` key and header spelling | | `403 insufficient_scope` | The OAuth token lacks the scope for that tool family (e.g. writes without `data:write`) — reconnect and grant the scope |