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/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 | 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); + }); +});