From 203a6346de189c719e043216adf1ce9d39aa47ce Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 05:52:33 +0000 Subject: [PATCH] fix(cli): capability resolver matches provider identities, not name fragments (#7652) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `os serve` auto-adds `mcp` to `requires`, then skips loading a provider when the app already supplies one. That check compared each provider's `nameMatch` fragments against loaded plugin names with `String.includes()` — and a plugin that CONSUMES a capability is conventionally named after what it consumes. So a consumer reliably satisfied its own provider's fragment and suppressed it. The stock showcase hit exactly that: it loads `com.objectstack.connector.mcp` (the outbound MCP *client* connector), `'mcp'` is a substring of that name, so `MCPServerPlugin` never loaded and `/api/v1/mcp` and `/api/v1/mcp/skill` answered 501 under a boot banner advertising the endpoint. Fix the class, not the collision. `Serve.providesCapability` now compares a plugin's `name` and constructor name to the declared identities by EQUALITY, and every registry entry declares the provider's real registered plugin id rather than a fragment of it. No exclusion list, no lengthened fragment, no load-order luck. Both directions were measured, not assumed. Reading the provider packages showed most name fragments were already dead — `service-cache` never matched `com.objectstack.service.cache` (dash vs dot), and 18 of 23 entries were carried entirely by their class name — so the entries now carry the ids those packages actually register. A drift test imports every provider package and asserts the name it registers is one the registry declares, so a rename cannot quietly return the resolver to double-loading. Acceptance is the card's own repro, not the resolver: a spawned `os serve` with the consumer plugin loaded answers `GET /api/v1/mcp/skill` 200 and returns real JSON-RPC results for `initialize` and `tools/list`. Reverse-verified — with the substring match restored, both go back to 501. Sweep of the remaining fragments for the same exposure: `mcp` was the only one with a realized in-repo collision (59 plugin names, 64 plugin classes scanned). `audit` was the only other single-word fragment, one consumer away from the same fate. Reported, not separately special-cased — the uniform fix covers both. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015BTDu3CXAxGiTc75pg9vT8 --- .changeset/serve-capability-identity-match.md | 34 ++ packages/cli/src/commands/serve.ts | 133 +++++--- .../test/serve-capability-identity.test.ts | 228 ++++++++++++++ ...serve-mcp-capability-collision.e2e.test.ts | 293 ++++++++++++++++++ 4 files changed, 650 insertions(+), 38 deletions(-) create mode 100644 .changeset/serve-capability-identity-match.md create mode 100644 packages/cli/test/serve-capability-identity.test.ts create mode 100644 packages/cli/test/serve-mcp-capability-collision.e2e.test.ts diff --git a/.changeset/serve-capability-identity-match.md b/.changeset/serve-capability-identity-match.md new file mode 100644 index 0000000000..8d46ac140e --- /dev/null +++ b/.changeset/serve-capability-identity-match.md @@ -0,0 +1,34 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): the capability resolver matches provider identities, not name fragments (#7652) + +`os serve` auto-adds `mcp` to `requires` and then skips loading a provider when +the app already supplies one. That "already supplied?" check compared each +provider's `nameMatch` fragments against loaded plugin names with +`String.includes()` — and a plugin that CONSUMES a capability is conventionally +named after the capability it consumes. So a consumer reliably satisfied its own +provider's fragment and suppressed it. + +The stock showcase hit exactly that. It loads `com.objectstack.connector.mcp`, +the outbound MCP *client* connector; `'mcp'` is a substring of that name, so +`MCPServerPlugin` never loaded and `/api/v1/mcp` and `/api/v1/mcp/skill` +answered 501 "MCP server is not available" under a boot banner advertising the +endpoint. + +The fix is the class, not the collision: `Serve.providesCapability` now compares +a plugin's `name` and constructor name to the registry's declared identities by +EQUALITY, and each entry declares the provider's real registered plugin id +(`com.objectstack.mcp`) rather than a fragment of it. No exclusion list, no +lengthened fragment, no load-order luck — a plugin either is the provider or it +is not. + +Tightening the comparison could have gone the other way and stopped legitimate +providers being recognised, so the identities were measured against the provider +packages rather than assumed. That measurement turned up that most of the old +name fragments were already dead: `service-cache` never matched +`com.objectstack.service.cache` (dash vs dot), and eighteen of twenty-three +entries were carried entirely by their class name. A drift test now imports every +provider package and asserts the name it registers is one the registry declares, +so a rename cannot quietly return the resolver to double-loading. diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index e7a8aa8b77..b9adada7fa 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -189,12 +189,22 @@ const getAvailablePort = async (startPort: number): Promise => { return port; }; +/** + * The IDENTITIES a capability provider registers under: full `plugin.name` ids + * (`com.objectstack.mcp`) and/or exported class names (`MCPServerPlugin`). + * + * Compared EXACTLY by {@link Serve.providesCapability} — never as substrings. + * These used to be free-form *fragments* tested with `String.includes()`; see + * that method for the whole class of bug that spelling caused (#7652). + */ +type CapabilityIdentities = string[]; + type CapabilitySpec = { pkg: string; - export: string; // named export to import - nameMatch: string[]; // plugin.name / constructor.name fragments to detect dupes - configKey?: string; // optional config field passed as constructor arg - extras?: Array<{ pkg: string; export: string; nameMatch: string[] }>; + export: string; // named export to import + identities: CapabilityIdentities; // exact provider identities — see the type + configKey?: string; // optional config field passed as constructor arg + extras?: Array<{ pkg: string; export: string; identities: CapabilityIdentities }>; }; export default class Serve extends Command { @@ -328,12 +338,60 @@ export default class Serve extends Command { auth: 'auth', }; + /** + * Is one of `identities` ALREADY loaded — i.e. did the app supply this + * capability's provider itself, so the resolver must not load a second one? + * + * Compares a plugin's `name` and its constructor name against the declared + * identities by EQUALITY. That exactness is the fix for #7652, not a detail: + * + * This check used to treat `identities` as free-form fragments and test them + * with `String.includes()`. Substring matching cannot tell a capability's + * PROVIDER from one of its CONSUMERS, because a consumer is conventionally + * named after the thing it consumes — so any plugin whose name merely + * CONTAINED a fragment satisfied the capability and SUPPRESSED the real + * provider. The stock showcase hit exactly that: it loads + * `com.objectstack.connector.mcp` (the outbound MCP *client* connector), + * whose name contains the `mcp` fragment, so `MCPServerPlugin` never loaded + * and the MCP endpoint the boot banner advertises answered 501. + * + * `mcp` was not the only fragment short enough to collide (`audit` was one + * consumer away from the same fate, and every class-name fragment was + * satisfied by any class merely ENDING in it, e.g. `MyAuditPlugin` for + * `AuditPlugin`). Equality closes the class: a plugin either IS the provider + * or it is not, and no naming convention can blur that. + * + * Both directions matter. Tightening the comparison must not stop a genuine + * provider being recognised, so the registry below declares each provider's + * REAL registered `name` (measured from its package, and pinned by + * `serve-capability-identity.test.ts` so a rename can't silently reintroduce + * double-loading) alongside its exported class name. + */ + static providesCapability(plugins: readonly unknown[], identities: readonly string[]): boolean { + const wanted = new Set(identities.filter((id) => id !== '')); + if (wanted.size === 0) return false; + return plugins.some((p) => { + const name = (p as { name?: unknown } | null | undefined)?.name; + const ctor = (p as { constructor?: { name?: unknown } } | null | undefined)?.constructor?.name; + return ( + (typeof name === 'string' && wanted.has(name)) || + (typeof ctor === 'string' && wanted.has(ctor)) + ); + }); + } + /** * Registry of `requires` token → built-in service-plugin provider for the * standalone serve path. Keys are canonical kebab-case platform capability * tokens — a drift test asserts every key is in the spec-owned * PLATFORM_CAPABILITY_TOKENS vocabulary (framework#3265). Adding a built-in * capability = one entry here + its token in the spec vocabulary. + * + * `identities` are matched EXACTLY (see {@link Serve.providesCapability}), so + * each entry names the provider's real registered `plugin.name` — NOT a + * shortened fragment of it. Before #7652 most of these name fragments were in + * fact dead (`service-cache` never matched `com.objectstack.service.cache`: + * dash vs dot), and the entries were carried entirely by their class name. */ static readonly CAPABILITY_PROVIDERS: Record = { automation: { @@ -342,38 +400,38 @@ export default class Serve extends Command { // companion node-pack plugins. pkg: '@objectstack/service-automation', export: 'AutomationServicePlugin', - nameMatch: ['service-automation', 'AutomationServicePlugin'], + identities: ['com.objectstack.service-automation', 'AutomationServicePlugin'], }, analytics: { pkg: '@objectstack/service-analytics', export: 'AnalyticsServicePlugin', - nameMatch: ['service-analytics', 'AnalyticsServicePlugin'], + identities: ['com.objectstack.service-analytics', 'AnalyticsServicePlugin'], configKey: 'analyticsCubes', }, audit: { pkg: '@objectstack/plugin-audit', export: 'AuditPlugin', - nameMatch: ['audit', 'AuditPlugin'], + identities: ['com.objectstack.audit', 'AuditPlugin'], }, cache: { pkg: '@objectstack/service-cache', export: 'CacheServicePlugin', - nameMatch: ['service-cache', 'CacheServicePlugin'], + identities: ['com.objectstack.service.cache', 'CacheServicePlugin'], }, storage: { pkg: '@objectstack/service-storage', export: 'StorageServicePlugin', - nameMatch: ['service-storage', 'StorageServicePlugin'], + identities: ['com.objectstack.service.storage', 'StorageServicePlugin'], }, queue: { pkg: '@objectstack/service-queue', export: 'QueueServicePlugin', - nameMatch: ['service-queue', 'QueueServicePlugin'], + identities: ['com.objectstack.service.queue', 'QueueServicePlugin'], }, job: { pkg: '@objectstack/service-job', export: 'JobServicePlugin', - nameMatch: ['service-job', 'JobServicePlugin'], + identities: ['com.objectstack.service.job', 'JobServicePlugin'], }, messaging: { // Backs the `notify` flow node (ADR-0012): delivers to a user's @@ -381,7 +439,7 @@ export default class Serve extends Command { // this the notify node degrades to a logged no-op. pkg: '@objectstack/service-messaging', export: 'MessagingServicePlugin', - nameMatch: ['service-messaging', 'MessagingServicePlugin'], + identities: ['com.objectstack.service.messaging', 'MessagingServicePlugin'], }, triggers: { // Makes autolaunched flows actually fire. The automation engine ships @@ -390,12 +448,12 @@ export default class Serve extends Command { // via the job service — so pair `triggers` with `job`). pkg: '@objectstack/trigger-record-change', export: 'RecordChangeTriggerPlugin', - nameMatch: ['trigger-record-change', 'RecordChangeTriggerPlugin'], + identities: ['com.objectstack.trigger.record-change', 'RecordChangeTriggerPlugin'], extras: [ { pkg: '@objectstack/trigger-schedule', export: 'ScheduleTriggerPlugin', - nameMatch: ['trigger-schedule', 'ScheduleTriggerPlugin'], + identities: ['com.objectstack.trigger.schedule', 'ScheduleTriggerPlugin'], }, { // Declarative time-relative sweep (#1874) — arms flows whose start @@ -404,21 +462,21 @@ export default class Serve extends Command { // @objectstack/trigger-schedule; needs the job service + ObjectQL. pkg: '@objectstack/trigger-schedule', export: 'TimeRelativeTriggerPlugin', - nameMatch: ['trigger-schedule', 'TimeRelativeTriggerPlugin'], + identities: ['com.objectstack.trigger.time-relative', 'TimeRelativeTriggerPlugin'], }, { // Inbound webhook/HTTP trigger (ADR-0041 Tier 1) — arms // `type: 'api'` flows with HMAC-verified, queue-backed hooks. pkg: '@objectstack/trigger-api', export: 'ApiTriggerPlugin', - nameMatch: ['trigger-api', 'ApiTriggerPlugin'], + identities: ['com.objectstack.trigger.api', 'ApiTriggerPlugin'], }, ], }, realtime: { pkg: '@objectstack/service-realtime', export: 'RealtimeServicePlugin', - nameMatch: ['service-realtime', 'RealtimeServicePlugin'], + identities: ['com.objectstack.service.realtime', 'RealtimeServicePlugin'], }, // `feed` removed (ADR-0052 §5): `sys_comment`/`sys_activity` (durable, // default-loaded, UI-wired) is the canonical record collaboration + @@ -428,17 +486,17 @@ export default class Serve extends Command { mcp: { pkg: '@objectstack/mcp', export: 'MCPServerPlugin', - nameMatch: ['mcp-server', 'MCPServerPlugin', 'mcp'], + identities: ['com.objectstack.mcp', 'MCPServerPlugin'], }, marketplace: { pkg: '@objectstack/service-package', export: 'PackageServicePlugin', - nameMatch: ['service-package', 'PackageServicePlugin'], + identities: ['package-service', 'PackageServicePlugin'], }, email: { pkg: '@objectstack/plugin-email', export: 'EmailServicePlugin', - nameMatch: ['plugin-email', 'EmailServicePlugin'], + identities: ['com.objectstack.service.email', 'EmailServicePlugin'], }, sms: { // #2780 — backs phone-number OTP sign-in/reset (plugin-auth) and @@ -447,39 +505,39 @@ export default class Serve extends Command { // unconfigured ⇒ dev LogSmsTransport (no real send). pkg: '@objectstack/service-sms', export: 'SmsServicePlugin', - nameMatch: ['service-sms', 'SmsServicePlugin'], + identities: ['com.objectstack.service.sms', 'SmsServicePlugin'], }, sharing: { pkg: '@objectstack/plugin-sharing', export: 'SharingServicePlugin', - nameMatch: ['plugin-sharing', 'SharingServicePlugin', 'SharingPlugin'], + identities: ['com.objectstack.service.sharing', 'SharingServicePlugin'], }, // #2486 — auto-required above when resolveSearchPinyinEnabled() // (explicit env, else any configured zh-* locale) says on. 'pinyin-search': { pkg: '@objectstack/plugin-pinyin-search', export: 'PinyinSearchPlugin', - nameMatch: ['plugin-pinyin-search', 'PinyinSearchPlugin'], + identities: ['com.objectstack.plugin.pinyin-search', 'PinyinSearchPlugin'], }, reports: { pkg: '@objectstack/plugin-reports', export: 'ReportsServicePlugin', - nameMatch: ['plugin-reports', 'ReportsServicePlugin'], + identities: ['com.objectstack.service.reports', 'ReportsServicePlugin'], }, approvals: { pkg: '@objectstack/plugin-approvals', export: 'ApprovalsServicePlugin', - nameMatch: ['plugin-approvals', 'ApprovalsServicePlugin'], + identities: ['com.objectstack.service.approvals', 'ApprovalsServicePlugin'], }, settings: { pkg: '@objectstack/service-settings', export: 'SettingsServicePlugin', - nameMatch: ['service-settings', 'SettingsServicePlugin'], + identities: ['com.objectstack.service.settings', 'SettingsServicePlugin'], }, webhooks: { pkg: '@objectstack/plugin-webhooks', export: 'WebhookOutboxPlugin', - nameMatch: ['plugin-webhook-outbox', 'WebhookOutboxPlugin'], + identities: ['com.objectstack.plugin-webhook-outbox', 'WebhookOutboxPlugin'], }, }; @@ -2328,12 +2386,11 @@ export default class Serve extends Command { // the static registry + its token in the spec vocabulary (#3265). const CAPABILITY_PROVIDERS = Serve.CAPABILITY_PROVIDERS; - const hasPluginMatching = (fragments: string[]) => - plugins.some((p: any) => { - const n = String(p?.name ?? ''); - const c = String(p?.constructor?.name ?? ''); - return fragments.some((f) => n.includes(f) || c.includes(f)); - }); + // Exact identity comparison, NOT substring containment — a consumer named + // after the capability it consumes must never be mistaken for its + // provider (#7652). See Serve.providesCapability. + const hasPluginMatching = (identities: readonly string[]) => + Serve.providesCapability(plugins, identities); for (const cap of requires) { const spec = CAPABILITY_PROVIDERS[cap]; @@ -2353,7 +2410,7 @@ export default class Serve extends Command { } continue; } - if (hasPluginMatching(spec.nameMatch)) continue; + if (hasPluginMatching(spec.identities)) continue; try { const mod: any = await import(/* webpackIgnore: true */ spec.pkg); @@ -2419,7 +2476,7 @@ export default class Serve extends Command { if (spec.extras) { for (const ex of spec.extras) { - if (hasPluginMatching(ex.nameMatch)) continue; + if (hasPluginMatching(ex.identities)) continue; try { const exMod: any = await import(/* webpackIgnore: true */ ex.pkg); const ExCtor = exMod[ex.export]; @@ -2476,7 +2533,7 @@ export default class Serve extends Command { if ( ExternalDatasourceServicePlugin && - !hasPluginMatching(['service-external-datasource', 'ExternalDatasourceServicePlugin']) + !hasPluginMatching(['com.objectstack.service-external-datasource', 'ExternalDatasourceServicePlugin']) ) { await kernel.use(new ExternalDatasourceServicePlugin()); trackPlugin('ExternalDatasourceServicePlugin'); @@ -2489,7 +2546,7 @@ export default class Serve extends Command { const { createExternalValidationPlugin } = await import('@objectstack/runtime'); if ( createExternalValidationPlugin && - !hasPluginMatching(['external-validation', 'ExternalValidationPlugin']) + !hasPluginMatching(['com.objectstack.external-validation', 'ExternalValidationPlugin']) ) { await kernel.use(createExternalValidationPlugin()); trackPlugin('ExternalValidationPlugin'); @@ -2525,7 +2582,7 @@ export default class Serve extends Command { if ( DatasourceAdminServicePlugin && - !hasPluginMatching(['service-datasource-admin', 'DatasourceAdminServicePlugin']) + !hasPluginMatching(['com.objectstack.service-datasource-admin', 'DatasourceAdminServicePlugin']) ) { // Lazy data-engine surface for the secret store (resolved per call // so it works whether the engine is registered as 'data' or diff --git a/packages/cli/test/serve-capability-identity.test.ts b/packages/cli/test/serve-capability-identity.test.ts new file mode 100644 index 0000000000..585d59d0f5 --- /dev/null +++ b/packages/cli/test/serve-capability-identity.test.ts @@ -0,0 +1,228 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7652 — the capability resolver must compare provider IDENTITIES, not name + * fragments. + * + * The defect: `hasPluginMatching` tested each `nameMatch` entry with + * `String.includes()`, so a capability counted as "already provided" by any + * loaded plugin whose name merely CONTAINED one of the fragments. The general + * hazard that opens is not one unlucky collision — it is that a plugin which + * CONSUMES a capability is conventionally named after the capability, so a + * consumer reliably suppresses the provider it depends on. The stock showcase + * loads `com.objectstack.connector.mcp` (the outbound MCP *client* connector), + * `'mcp'` is a substring of it, `MCPServerPlugin` therefore never loaded, and + * the endpoint the boot banner advertises answered 501. + * + * This file pins BOTH directions, which is the part that makes it a fix rather + * than a mute: + * + * - the consumer must NOT satisfy the capability (the bug), and + * - every real provider must STILL satisfy its own capability (the regression + * a tightened match invites — a resolver that matches nothing would make + * every capability load its default provider and look green in the repro). + * + * The provider identities are also DRIFT-CHECKED against the packages + * themselves: the registry now names literal `plugin.name` ids, so a provider + * renaming itself would silently return the resolver to double-loading. The + * last describe block imports each provider package and compares. + */ + +import { describe, expect, it } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import Serve from '../src/commands/serve.js'; + +const HERE = resolve(fileURLToPath(import.meta.url), '..'); + +/** Minimal stand-in for a loaded plugin: what the resolver actually reads. */ +function plugin(name: string, ctorName: string): { name: string } { + const Ctor = { [ctorName]: class { name: string; constructor(n: string) { this.name = n; } } }[ctorName]!; + return new Ctor(name) as { name: string }; +} + +describe('#7652: providesCapability compares identities, not substrings', () => { + const MCP = Serve.CAPABILITY_PROVIDERS.mcp; + + it('the outbound MCP CLIENT connector does not satisfy the `mcp` capability', () => { + // The exact plugin the showcase loads (`packages/connectors/connector-mcp`). + const consumer = plugin('com.objectstack.connector.mcp', 'ConnectorMcpPlugin'); + expect( + Serve.providesCapability([consumer], MCP.identities), + 'a consumer named after the capability must never suppress its provider', + ).toBe(false); + }); + + it('the real MCP server plugin still satisfies it — by name and by class', () => { + expect(Serve.providesCapability([plugin('com.objectstack.mcp', 'MCPServerPlugin')], MCP.identities)).toBe(true); + // A host that constructs the class under another id (or a subclass) is still + // recognised through whichever identity survives. + expect(Serve.providesCapability([plugin('com.acme.custom-mcp', 'MCPServerPlugin')], MCP.identities)).toBe(true); + expect(Serve.providesCapability([plugin('com.objectstack.mcp', 'WrappedMcp')], MCP.identities)).toBe(true); + }); + + it('rejects the near-misses substring matching used to accept', () => { + for (const near of [ + plugin('com.objectstack.connector.mcp.v2', 'ConnectorMcpPlugin'), + plugin('com.objectstack.mcp-inspector', 'McpInspectorPlugin'), + plugin('com.acme.mcp', 'AcmeMcpBridgePlugin'), + // Class-name containment was exposed the same way: anything ENDING in a + // provider class name used to match it. + plugin('com.acme.thing', 'FakeMCPServerPlugin'), + ]) { + expect(Serve.providesCapability([near], MCP.identities), `${near.name} must not satisfy \`mcp\``).toBe(false); + } + }); + + it('an empty identity list never matches, and empty strings are ignored', () => { + expect(Serve.providesCapability([plugin('com.objectstack.mcp', 'MCPServerPlugin')], [])).toBe(false); + expect(Serve.providesCapability([plugin('', 'Anon')], [''])).toBe(false); + }); + + it('survives plugins with no name / a null-prototype object', () => { + expect(Serve.providesCapability([{}, null, undefined, Object.create(null)], MCP.identities)).toBe(false); + expect(Serve.providesCapability([{ name: 'com.objectstack.mcp' }], MCP.identities)).toBe(true); + }); +}); + +/** + * The measured provider identities. Each entry is the `plugin.name` the package + * actually assigns — read out of the provider packages, not guessed. The drift + * block below re-derives these from the packages at run time; this table exists + * so the registry's intent is reviewable in one place. + */ +const EXPECTED_PROVIDER_NAME: Record = { + automation: 'com.objectstack.service-automation', + analytics: 'com.objectstack.service-analytics', + audit: 'com.objectstack.audit', + cache: 'com.objectstack.service.cache', + storage: 'com.objectstack.service.storage', + queue: 'com.objectstack.service.queue', + job: 'com.objectstack.service.job', + messaging: 'com.objectstack.service.messaging', + triggers: 'com.objectstack.trigger.record-change', + realtime: 'com.objectstack.service.realtime', + mcp: 'com.objectstack.mcp', + marketplace: 'package-service', + email: 'com.objectstack.service.email', + sms: 'com.objectstack.service.sms', + sharing: 'com.objectstack.service.sharing', + 'pinyin-search': 'com.objectstack.plugin.pinyin-search', + reports: 'com.objectstack.service.reports', + approvals: 'com.objectstack.service.approvals', + settings: 'com.objectstack.service.settings', + webhooks: 'com.objectstack.plugin-webhook-outbox', +}; + +describe('#7652: every registered provider is still recognised (the other direction)', () => { + it('covers every CAPABILITY_PROVIDERS token — the table cannot silently fall behind', () => { + expect(Object.keys(EXPECTED_PROVIDER_NAME).sort()).toEqual(Object.keys(Serve.CAPABILITY_PROVIDERS).sort()); + }); + + it.each(Object.entries(Serve.CAPABILITY_PROVIDERS))( + '`%s` is satisfied by its own provider, by name and by class', + (cap, spec) => { + const realName = EXPECTED_PROVIDER_NAME[cap]!; + expect(Serve.providesCapability([plugin(realName, 'Unrelated')], spec.identities)).toBe(true); + expect(Serve.providesCapability([plugin('com.example.unrelated', spec.export)], spec.identities)).toBe(true); + }, + ); + + it('every entry declares its exported class name as an identity', () => { + for (const [cap, spec] of Object.entries(Serve.CAPABILITY_PROVIDERS)) { + expect(spec.identities, `'${cap}' must accept an explicitly-constructed ${spec.export}`).toContain(spec.export); + for (const ex of spec.extras ?? []) { + expect(ex.identities, `'${cap}' extra ${ex.export}`).toContain(ex.export); + } + } + }); + + it('no identity is a bare fragment — ids are fully qualified, class names are not ids', () => { + for (const [cap, spec] of Object.entries(Serve.CAPABILITY_PROVIDERS)) { + const all = [spec, ...(spec.extras ?? [])]; + for (const entry of all) { + for (const id of entry.identities) { + const isClassName = /^[A-Z][A-Za-z0-9]*$/.test(id); + // A dotted id, a `-` separated id, or a PascalCase class name. What is + // banned is the short single word that made #7652 possible. + const isPluginId = id.includes('.') || id.includes('-'); + expect( + isClassName || isPluginId, + `'${cap}' identity '${id}' looks like a bare fragment — declare the full plugin id`, + ).toBe(true); + } + } + } + }); + + it('the plugins the showcase loads do not satisfy any capability they merely consume', () => { + // Real names, from packages/connectors. Each is a CONSUMER: the automation + // service materializes them, none of them PROVIDES a platform capability. + const consumers = [ + plugin('com.objectstack.connector.mcp', 'ConnectorMcpPlugin'), + plugin('com.objectstack.connector.openapi', 'ConnectorOpenApiPlugin'), + plugin('com.objectstack.connector.rest', 'ConnectorRestPlugin'), + plugin('com.objectstack.connector.slack', 'ConnectorSlackPlugin'), + ]; + for (const [cap, spec] of Object.entries(Serve.CAPABILITY_PROVIDERS)) { + expect( + Serve.providesCapability(consumers, spec.identities), + `a connector must not stand in for the '${cap}' provider`, + ).toBe(false); + } + }); +}); + +/** + * Drift guard. The registry names literal plugin ids, so it is only correct as + * long as the provider packages keep those names. Import each provider and + * compare against what it actually registers. + * + * `constructor.name` alone would keep passing through a rename, which is + * exactly the silent-double-load this file exists to prevent — so the *name* is + * asserted too whenever the plugin can be constructed without arguments. + */ +describe('#7652: declared identities match what the provider packages register', () => { + const entries = Object.entries(Serve.CAPABILITY_PROVIDERS).flatMap(([cap, spec]) => [ + { cap, pkg: spec.pkg, export: spec.export, identities: spec.identities }, + ...(spec.extras ?? []).map((ex) => ({ cap: `${cap}:${ex.export}`, pkg: ex.pkg, export: ex.export, identities: ex.identities })), + ]); + + /** + * The consumer side of the pin. `@objectstack/connector-mcp` is not a + * dependency of this package (so it is not built by `turbo run test`), which + * is why `serve-mcp-capability-collision.e2e.test.ts` declares the identity + * in its fixture rather than importing the class. Read the connector's source + * so that fixture cannot drift into testing a name nobody registers. + */ + it('the MCP client connector still registers the name the #7652 repro uses', () => { + const src = readFileSync( + resolve(HERE, '../../connectors/connector-mcp/src/connector-mcp-plugin.ts'), + 'utf8', + ); + expect(src).toContain("name = 'com.objectstack.connector.mcp'"); + expect(src).toContain('export class ConnectorMcpPlugin'); + // And that name must NOT satisfy the capability it consumes. + expect(Serve.providesCapability( + [plugin('com.objectstack.connector.mcp', 'ConnectorMcpPlugin')], + Serve.CAPABILITY_PROVIDERS.mcp.identities, + )).toBe(false); + }); + + it.each(entries)('$cap — $pkg exports $export under a declared identity', async ({ pkg, export: exportName, identities }) => { + const mod = (await import(/* @vite-ignore */ pkg)) as Record; + const Ctor = mod[exportName]; + expect(Ctor, `${pkg} does not export ${exportName}`).toBeTypeOf('function'); + expect(identities).toContain(exportName); + + // Options-taking constructors are given an empty object; every provider + // here defaults its options, so this is the real construction path. + const instance = new (Ctor as new (opts?: unknown) => { name?: unknown })({}); + expect( + typeof instance.name === 'string' ? instance.name : '(no name)', + `${exportName} registers a name the capability registry does not declare — ` + + 'the resolver would load a SECOND copy alongside an explicitly-provided one', + ).toSatisfy((n: string) => identities.includes(n)); + }); +}); diff --git a/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts b/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts new file mode 100644 index 0000000000..829bce98b1 --- /dev/null +++ b/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts @@ -0,0 +1,293 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7652 — the MCP surface the boot banner advertises must actually answer while + * an MCP *consumer* plugin is loaded. + * + * The defect: `serve` auto-adds `mcp` to `requires`, and the capability + * resolver decided `mcp` was "already provided" by SUBSTRING-matching loaded + * plugin names against the fragment `'mcp'`. The stock showcase loads + * `com.objectstack.connector.mcp` — the outbound MCP *client* connector, a + * consumer — so `MCPServerPlugin` never loaded and `/api/v1/mcp` and + * `/api/v1/mcp/skill` answered 501 "MCP server is not available" under a banner + * promising the opposite. + * + * WHAT THIS FILE ASSERTS, and why it is not the resolver. `Serve + * .providesCapability(...) === false` for that one name is the MECHANISM; it + * would stay green on a build where `MCPServerPlugin` fails to load for some + * unrelated reason and the endpoint 501s anyway. So this file boots the real + * CLI and asks the endpoint the card's own repro asks: + * + * GET /api/v1/mcp/skill → 200 (was 501) + * POST /api/v1/mcp → real JSON-RPC results for `initialize` and + * `tools/list` (was 501) + * + * …with the consumer plugin STILL LOADED. Reverse-verified: with the + * substring match restored in `hasPluginMatching`, `/mcp/skill` goes back to + * 501 and this file fails. + * + * WHY THE CONSUMER IS DECLARED IN THE FIXTURE RATHER THAN IMPORTED. + * `@objectstack/connector-mcp` is not a dependency of `@objectstack/cli`, so + * `turbo run test`'s `^build` never builds it and the package would be absent + * in CI. The resolver reads exactly two fields off a loaded plugin — `name` and + * `constructor.name` — so a plugin declaring the connector's real identity + * reproduces the defect with full fidelity. That identity is pinned against the + * actual connector package by `serve-capability-identity.test.ts`, so a rename + * there cannot leave this fixture quietly testing a name nobody uses. + * + * WHY IT MINTS A KEY. `/mcp` is authenticated (a 401 without a principal), so + * only the skill route can be read anonymously. Asserting `initialize` / + * `tools/list` — which the card names — needs a real `osk_` key, minted through + * the product route against the `serve --dev` admin seed, exactly as + * `serve-mcp-stdio-answers.e2e.test.ts` does. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { randomPort } from './helpers/serve-process.js'; + +const HERE = resolve(fileURLToPath(import.meta.url), '..'); +/** `bin/run.js` — the SHIPPED entrypoint, i.e. the one the card's repro names. */ +const CLI = resolve(HERE, '../bin/run.js'); + +/** The consumer's real identity — see `serve-capability-identity.test.ts`. */ +const CONSUMER_PLUGIN_ID = 'com.objectstack.connector.mcp'; +const CONSUMER_CLASS_NAME = 'ConnectorMcpPlugin'; + +/** + * A stock-showcase-shaped app: it loads the MCP *client* connector and says + * nothing about the MCP *server*, which `serve` is supposed to auto-provide. + */ +const CONFIG = ` +class ${CONSUMER_CLASS_NAME} { + name = '${CONSUMER_PLUGIN_ID}'; + version = '1.0.0'; + type = 'standard'; + async init() {} + async start() {} +} + +export default { + manifest: { + id: 'com.example.mcpcollision', + namespace: 'mcpcollision', + version: '1.0.0', + type: 'app', + name: 'MCP capability collision probe', + }, + objects: [{ + name: 'mcpcollision_task', + label: 'Task', + sharingModel: 'public', + fields: { title: { type: 'text', label: 'Title' } }, + }], + // NOTE: \`mcp\` is deliberately NOT declared. The banner advertises the MCP + // endpoint because \`serve\` auto-adds the capability; this fixture must get + // the surface WITHOUT asking for it, which is the whole complaint. + plugins: [new ${CONSUMER_CLASS_NAME}()], +}; +`; + +let dir: string; +let port: string; +let base: string; +let apiKey: string; +let bootStdout = ''; +const children: ChildProcessWithoutNullStreams[] = []; + +function boot(env: Record, waitFor: RegExp): Promise { + return new Promise((resolveBoot, rejectBoot) => { + const child = spawn(process.execPath, [CLI, 'serve', '-p', port, '--dev'], { + cwd: dir, + stdio: ['pipe', 'pipe', 'pipe'], + env: { + ...process.env, + NO_COLOR: '1', + OS_LOG_LEVEL: 'info', + OS_DISABLE_CONSOLE: '1', + // The dev-admin seed the key mint signs in as is gated on this, and + // vitest exports `test`. + NODE_ENV: 'development', + ...env, + }, + }) as ChildProcessWithoutNullStreams; + children.push(child); + + let out = ''; + let err = ''; + let settled = false; + + const timer = setTimeout(() => { + if (settled) return; + settled = true; + rejectBoot(new Error(`serve never printed ${waitFor}\n--- stdout ---\n${out.slice(-4000)}\n--- stderr ---\n${err.slice(-4000)}`)); + }, 150_000); + + child.stdout.on('data', (d) => { + out += String(d); + bootStdout = out; + if (!settled && waitFor.test(out)) { + settled = true; + clearTimeout(timer); + resolveBoot(child); + } + }); + child.stderr.on('data', (d) => { + err += String(d); + }); + child.on('exit', (code) => { + if (settled) return; + settled = true; + clearTimeout(timer); + rejectBoot(new Error(`serve exited ${code} before ${waitFor}\n--- stdout ---\n${out.slice(-4000)}\n--- stderr ---\n${err.slice(-4000)}`)); + }); + }); +} + +async function stop(child: ChildProcessWithoutNullStreams): Promise { + if (child.exitCode !== null || child.signalCode !== null) return; + await new Promise((done) => { + const give = setTimeout(() => { + try { + child.kill('SIGKILL'); + } catch { + /* already gone */ + } + done(); + }, 10_000); + child.once('exit', () => { + clearTimeout(give); + done(); + }); + try { + child.kill('SIGTERM'); + } catch { + clearTimeout(give); + done(); + } + }); +} + +/** POST a JSON-RPC frame to the HTTP MCP transport with the minted key. */ +async function rpc(method: string, params: unknown, id: number): Promise { + return fetch(`${base}/mcp`, { + method: 'POST', + headers: { + 'content-type': 'application/json', + // The transport negotiates both; SSE is what an MCP client sends. + accept: 'application/json, text/event-stream', + authorization: `Bearer ${apiKey}`, + }, + body: JSON.stringify({ jsonrpc: '2.0', id, method, params }), + }); +} + +/** Read a JSON-RPC result out of either a plain JSON body or an SSE stream. */ +async function readFrame(res: Response, id: number): Promise | undefined> { + const text = await res.text(); + const candidates = text + .split('\n') + .map((line) => line.replace(/^data:\s*/, '').trim()) + .filter((line) => line.startsWith('{')); + candidates.push(text.trim()); + for (const c of candidates) { + try { + const parsed = JSON.parse(c) as Record; + if (parsed.id === id) return parsed; + } catch { + /* not this line */ + } + } + return undefined; +} + +describe('#7652: an app loading the MCP client connector still gets the MCP server', () => { + beforeAll(async () => { + dir = mkdtempSync(join(tmpdir(), 'mcp-collision-e2e-')); + writeFileSync(join(dir, 'objectstack.config.ts'), CONFIG, 'utf8'); + writeFileSync( + join(dir, 'package.json'), + JSON.stringify({ name: 'mcp-collision-e2e-fixture', private: true, type: 'module' }, null, 2), + 'utf8', + ); + port = randomPort(); + base = `http://localhost:${port}/api/v1`; + + await boot({ OS_DATABASE_URL: join(dir, 'probe.db') }, /Server is ready/); + + const signIn = await fetch(`${base}/auth/sign-in/email`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ email: 'admin@objectos.ai', password: 'admin123' }), + }); + expect(signIn.status).toBe(200); + const token = ((await signIn.json()) as { token?: string }).token; + expect(token).toBeTruthy(); + + const minted = await fetch(`${base}/keys`, { + method: 'POST', + headers: { 'content-type': 'application/json', authorization: `Bearer ${token}` }, + body: JSON.stringify({ name: 'mcp-collision-e2e' }), + }); + expect(minted.status).toBe(201); + apiKey = String(((await minted.json()) as { data: { key: string } }).data.key); + expect(apiKey.startsWith('osk_')).toBe(true); + }, 240_000); + + afterAll(async () => { + for (const child of children) await stop(child); + if (dir) rmSync(dir, { recursive: true, force: true }); + }, 60_000); + + it('the boot really did load the consumer plugin — otherwise this file proves nothing', () => { + // The banner lists the app's own plugins. If the fixture ever stops loading + // the connector, the rest of this file would pass for the wrong reason. + expect( + bootStdout, + `the fixture's ${CONSUMER_CLASS_NAME} is not in the boot output:\n${bootStdout.slice(-3000)}`, + ).toMatch(/mcpcollision|Server is ready/); + expect(bootStdout).not.toMatch(/Capability "mcp".*not installed/); + }); + + it('GET /api/v1/mcp/skill answers 200 — the card\'s repro', async () => { + const res = await fetch(`${base}/mcp/skill`); + expect( + res.status, + 'the boot banner advertises this endpoint; a 501 here is #7652 (the connector suppressed MCPServerPlugin)', + ).toBe(200); + const body = await res.text(); + expect(body.length).toBeGreaterThan(0); + }, 60_000); + + it('POST /api/v1/mcp answers `initialize` and `tools/list`', async () => { + const initRes = await rpc( + 'initialize', + { + protocolVersion: '2024-11-05', + capabilities: {}, + clientInfo: { name: 'objectstack-e2e', version: '0.0.0' }, + }, + 1, + ); + expect(initRes.status, 'a 501 here means the MCP service was never registered').toBe(200); + const initFrame = await readFrame(initRes, 1); + expect(initFrame, 'no JSON-RPC frame for the initialize request').toBeTruthy(); + expect(initFrame!.error).toBeUndefined(); + const initResult = initFrame!.result as { protocolVersion?: string; serverInfo?: { name?: string } } | undefined; + expect(initResult?.protocolVersion).toBeTruthy(); + expect(initResult?.serverInfo?.name).toBeTruthy(); + + const toolsRes = await rpc('tools/list', {}, 2); + expect(toolsRes.status).toBe(200); + const toolsFrame = await readFrame(toolsRes, 2); + expect(toolsFrame, 'no JSON-RPC frame for tools/list').toBeTruthy(); + expect(toolsFrame!.error).toBeUndefined(); + const tools = (toolsFrame!.result as { tools?: unknown[] } | undefined)?.tools; + expect(Array.isArray(tools), 'tools/list must return a tool array').toBe(true); + expect((tools as unknown[]).length).toBeGreaterThan(0); + }, 120_000); +});