fix(runtime,rest): advertise mcp only when it is actually serveable (#4032)

* fix(runtime,rest): advertise `mcp` only when it is actually serveable

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011AvZj6cLX7APd7roh2eK4F

* docs(ai): record the enabled-but-unserveable MCP symptom

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011AvZj6cLX7APd7roh2eK4F

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.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.

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 |

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<boolean | null> {
+        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;

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

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