Skip to content

Commit 2daafe1

Browse files
hotlongclaude
andauthored
fix(cli): capability resolver matches provider identities, not name fragments (#7652) (#7935)
`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. Claude-Session: https://claude.ai/code/session_015BTDu3CXAxGiTc75pg9vT8 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 833ed84 commit 2daafe1

4 files changed

Lines changed: 650 additions & 38 deletions

File tree

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
fix(cli): the capability resolver matches provider identities, not name fragments (#7652)
6+
7+
`os serve` auto-adds `mcp` to `requires` and then skips loading a provider when
8+
the app already supplies one. That "already supplied?" check compared each
9+
provider's `nameMatch` fragments against loaded plugin names with
10+
`String.includes()` — and a plugin that CONSUMES a capability is conventionally
11+
named after the capability it consumes. So a consumer reliably satisfied its own
12+
provider's fragment and suppressed it.
13+
14+
The stock showcase hit exactly that. It loads `com.objectstack.connector.mcp`,
15+
the outbound MCP *client* connector; `'mcp'` is a substring of that name, so
16+
`MCPServerPlugin` never loaded and `/api/v1/mcp` and `/api/v1/mcp/skill`
17+
answered 501 "MCP server is not available" under a boot banner advertising the
18+
endpoint.
19+
20+
The fix is the class, not the collision: `Serve.providesCapability` now compares
21+
a plugin's `name` and constructor name to the registry's declared identities by
22+
EQUALITY, and each entry declares the provider's real registered plugin id
23+
(`com.objectstack.mcp`) rather than a fragment of it. No exclusion list, no
24+
lengthened fragment, no load-order luck — a plugin either is the provider or it
25+
is not.
26+
27+
Tightening the comparison could have gone the other way and stopped legitimate
28+
providers being recognised, so the identities were measured against the provider
29+
packages rather than assumed. That measurement turned up that most of the old
30+
name fragments were already dead: `service-cache` never matched
31+
`com.objectstack.service.cache` (dash vs dot), and eighteen of twenty-three
32+
entries were carried entirely by their class name. A drift test now imports every
33+
provider package and asserts the name it registers is one the registry declares,
34+
so a rename cannot quietly return the resolver to double-loading.

packages/cli/src/commands/serve.ts

Lines changed: 95 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -189,12 +189,22 @@ const getAvailablePort = async (startPort: number): Promise<number> => {
189189
return port;
190190
};
191191

192+
/**
193+
* The IDENTITIES a capability provider registers under: full `plugin.name` ids
194+
* (`com.objectstack.mcp`) and/or exported class names (`MCPServerPlugin`).
195+
*
196+
* Compared EXACTLY by {@link Serve.providesCapability} — never as substrings.
197+
* These used to be free-form *fragments* tested with `String.includes()`; see
198+
* that method for the whole class of bug that spelling caused (#7652).
199+
*/
200+
type CapabilityIdentities = string[];
201+
192202
type CapabilitySpec = {
193203
pkg: string;
194-
export: string; // named export to import
195-
nameMatch: string[]; // plugin.name / constructor.name fragments to detect dupes
196-
configKey?: string; // optional config field passed as constructor arg
197-
extras?: Array<{ pkg: string; export: string; nameMatch: string[] }>;
204+
export: string; // named export to import
205+
identities: CapabilityIdentities; // exact provider identities — see the type
206+
configKey?: string; // optional config field passed as constructor arg
207+
extras?: Array<{ pkg: string; export: string; identities: CapabilityIdentities }>;
198208
};
199209

200210
export default class Serve extends Command {
@@ -328,12 +338,60 @@ export default class Serve extends Command {
328338
auth: 'auth',
329339
};
330340

341+
/**
342+
* Is one of `identities` ALREADY loaded — i.e. did the app supply this
343+
* capability's provider itself, so the resolver must not load a second one?
344+
*
345+
* Compares a plugin's `name` and its constructor name against the declared
346+
* identities by EQUALITY. That exactness is the fix for #7652, not a detail:
347+
*
348+
* This check used to treat `identities` as free-form fragments and test them
349+
* with `String.includes()`. Substring matching cannot tell a capability's
350+
* PROVIDER from one of its CONSUMERS, because a consumer is conventionally
351+
* named after the thing it consumes — so any plugin whose name merely
352+
* CONTAINED a fragment satisfied the capability and SUPPRESSED the real
353+
* provider. The stock showcase hit exactly that: it loads
354+
* `com.objectstack.connector.mcp` (the outbound MCP *client* connector),
355+
* whose name contains the `mcp` fragment, so `MCPServerPlugin` never loaded
356+
* and the MCP endpoint the boot banner advertises answered 501.
357+
*
358+
* `mcp` was not the only fragment short enough to collide (`audit` was one
359+
* consumer away from the same fate, and every class-name fragment was
360+
* satisfied by any class merely ENDING in it, e.g. `MyAuditPlugin` for
361+
* `AuditPlugin`). Equality closes the class: a plugin either IS the provider
362+
* or it is not, and no naming convention can blur that.
363+
*
364+
* Both directions matter. Tightening the comparison must not stop a genuine
365+
* provider being recognised, so the registry below declares each provider's
366+
* REAL registered `name` (measured from its package, and pinned by
367+
* `serve-capability-identity.test.ts` so a rename can't silently reintroduce
368+
* double-loading) alongside its exported class name.
369+
*/
370+
static providesCapability(plugins: readonly unknown[], identities: readonly string[]): boolean {
371+
const wanted = new Set(identities.filter((id) => id !== ''));
372+
if (wanted.size === 0) return false;
373+
return plugins.some((p) => {
374+
const name = (p as { name?: unknown } | null | undefined)?.name;
375+
const ctor = (p as { constructor?: { name?: unknown } } | null | undefined)?.constructor?.name;
376+
return (
377+
(typeof name === 'string' && wanted.has(name)) ||
378+
(typeof ctor === 'string' && wanted.has(ctor))
379+
);
380+
});
381+
}
382+
331383
/**
332384
* Registry of `requires` token → built-in service-plugin provider for the
333385
* standalone serve path. Keys are canonical kebab-case platform capability
334386
* tokens — a drift test asserts every key is in the spec-owned
335387
* PLATFORM_CAPABILITY_TOKENS vocabulary (framework#3265). Adding a built-in
336388
* capability = one entry here + its token in the spec vocabulary.
389+
*
390+
* `identities` are matched EXACTLY (see {@link Serve.providesCapability}), so
391+
* each entry names the provider's real registered `plugin.name` — NOT a
392+
* shortened fragment of it. Before #7652 most of these name fragments were in
393+
* fact dead (`service-cache` never matched `com.objectstack.service.cache`:
394+
* dash vs dot), and the entries were carried entirely by their class name.
337395
*/
338396
static readonly CAPABILITY_PROVIDERS: Record<string, CapabilitySpec> = {
339397
automation: {
@@ -342,46 +400,46 @@ export default class Serve extends Command {
342400
// companion node-pack plugins.
343401
pkg: '@objectstack/service-automation',
344402
export: 'AutomationServicePlugin',
345-
nameMatch: ['service-automation', 'AutomationServicePlugin'],
403+
identities: ['com.objectstack.service-automation', 'AutomationServicePlugin'],
346404
},
347405
analytics: {
348406
pkg: '@objectstack/service-analytics',
349407
export: 'AnalyticsServicePlugin',
350-
nameMatch: ['service-analytics', 'AnalyticsServicePlugin'],
408+
identities: ['com.objectstack.service-analytics', 'AnalyticsServicePlugin'],
351409
configKey: 'analyticsCubes',
352410
},
353411
audit: {
354412
pkg: '@objectstack/plugin-audit',
355413
export: 'AuditPlugin',
356-
nameMatch: ['audit', 'AuditPlugin'],
414+
identities: ['com.objectstack.audit', 'AuditPlugin'],
357415
},
358416
cache: {
359417
pkg: '@objectstack/service-cache',
360418
export: 'CacheServicePlugin',
361-
nameMatch: ['service-cache', 'CacheServicePlugin'],
419+
identities: ['com.objectstack.service.cache', 'CacheServicePlugin'],
362420
},
363421
storage: {
364422
pkg: '@objectstack/service-storage',
365423
export: 'StorageServicePlugin',
366-
nameMatch: ['service-storage', 'StorageServicePlugin'],
424+
identities: ['com.objectstack.service.storage', 'StorageServicePlugin'],
367425
},
368426
queue: {
369427
pkg: '@objectstack/service-queue',
370428
export: 'QueueServicePlugin',
371-
nameMatch: ['service-queue', 'QueueServicePlugin'],
429+
identities: ['com.objectstack.service.queue', 'QueueServicePlugin'],
372430
},
373431
job: {
374432
pkg: '@objectstack/service-job',
375433
export: 'JobServicePlugin',
376-
nameMatch: ['service-job', 'JobServicePlugin'],
434+
identities: ['com.objectstack.service.job', 'JobServicePlugin'],
377435
},
378436
messaging: {
379437
// Backs the `notify` flow node (ADR-0012): delivers to a user's
380438
// channels (inbox by default → `sys_inbox_message` rows). Without
381439
// this the notify node degrades to a logged no-op.
382440
pkg: '@objectstack/service-messaging',
383441
export: 'MessagingServicePlugin',
384-
nameMatch: ['service-messaging', 'MessagingServicePlugin'],
442+
identities: ['com.objectstack.service.messaging', 'MessagingServicePlugin'],
385443
},
386444
triggers: {
387445
// Makes autolaunched flows actually fire. The automation engine ships
@@ -390,12 +448,12 @@ export default class Serve extends Command {
390448
// via the job service — so pair `triggers` with `job`).
391449
pkg: '@objectstack/trigger-record-change',
392450
export: 'RecordChangeTriggerPlugin',
393-
nameMatch: ['trigger-record-change', 'RecordChangeTriggerPlugin'],
451+
identities: ['com.objectstack.trigger.record-change', 'RecordChangeTriggerPlugin'],
394452
extras: [
395453
{
396454
pkg: '@objectstack/trigger-schedule',
397455
export: 'ScheduleTriggerPlugin',
398-
nameMatch: ['trigger-schedule', 'ScheduleTriggerPlugin'],
456+
identities: ['com.objectstack.trigger.schedule', 'ScheduleTriggerPlugin'],
399457
},
400458
{
401459
// Declarative time-relative sweep (#1874) — arms flows whose start
@@ -404,21 +462,21 @@ export default class Serve extends Command {
404462
// @objectstack/trigger-schedule; needs the job service + ObjectQL.
405463
pkg: '@objectstack/trigger-schedule',
406464
export: 'TimeRelativeTriggerPlugin',
407-
nameMatch: ['trigger-schedule', 'TimeRelativeTriggerPlugin'],
465+
identities: ['com.objectstack.trigger.time-relative', 'TimeRelativeTriggerPlugin'],
408466
},
409467
{
410468
// Inbound webhook/HTTP trigger (ADR-0041 Tier 1) — arms
411469
// `type: 'api'` flows with HMAC-verified, queue-backed hooks.
412470
pkg: '@objectstack/trigger-api',
413471
export: 'ApiTriggerPlugin',
414-
nameMatch: ['trigger-api', 'ApiTriggerPlugin'],
472+
identities: ['com.objectstack.trigger.api', 'ApiTriggerPlugin'],
415473
},
416474
],
417475
},
418476
realtime: {
419477
pkg: '@objectstack/service-realtime',
420478
export: 'RealtimeServicePlugin',
421-
nameMatch: ['service-realtime', 'RealtimeServicePlugin'],
479+
identities: ['com.objectstack.service.realtime', 'RealtimeServicePlugin'],
422480
},
423481
// `feed` removed (ADR-0052 §5): `sys_comment`/`sys_activity` (durable,
424482
// default-loaded, UI-wired) is the canonical record collaboration +
@@ -428,17 +486,17 @@ export default class Serve extends Command {
428486
mcp: {
429487
pkg: '@objectstack/mcp',
430488
export: 'MCPServerPlugin',
431-
nameMatch: ['mcp-server', 'MCPServerPlugin', 'mcp'],
489+
identities: ['com.objectstack.mcp', 'MCPServerPlugin'],
432490
},
433491
marketplace: {
434492
pkg: '@objectstack/service-package',
435493
export: 'PackageServicePlugin',
436-
nameMatch: ['service-package', 'PackageServicePlugin'],
494+
identities: ['package-service', 'PackageServicePlugin'],
437495
},
438496
email: {
439497
pkg: '@objectstack/plugin-email',
440498
export: 'EmailServicePlugin',
441-
nameMatch: ['plugin-email', 'EmailServicePlugin'],
499+
identities: ['com.objectstack.service.email', 'EmailServicePlugin'],
442500
},
443501
sms: {
444502
// #2780 — backs phone-number OTP sign-in/reset (plugin-auth) and
@@ -447,39 +505,39 @@ export default class Serve extends Command {
447505
// unconfigured ⇒ dev LogSmsTransport (no real send).
448506
pkg: '@objectstack/service-sms',
449507
export: 'SmsServicePlugin',
450-
nameMatch: ['service-sms', 'SmsServicePlugin'],
508+
identities: ['com.objectstack.service.sms', 'SmsServicePlugin'],
451509
},
452510
sharing: {
453511
pkg: '@objectstack/plugin-sharing',
454512
export: 'SharingServicePlugin',
455-
nameMatch: ['plugin-sharing', 'SharingServicePlugin', 'SharingPlugin'],
513+
identities: ['com.objectstack.service.sharing', 'SharingServicePlugin'],
456514
},
457515
// #2486 — auto-required above when resolveSearchPinyinEnabled()
458516
// (explicit env, else any configured zh-* locale) says on.
459517
'pinyin-search': {
460518
pkg: '@objectstack/plugin-pinyin-search',
461519
export: 'PinyinSearchPlugin',
462-
nameMatch: ['plugin-pinyin-search', 'PinyinSearchPlugin'],
520+
identities: ['com.objectstack.plugin.pinyin-search', 'PinyinSearchPlugin'],
463521
},
464522
reports: {
465523
pkg: '@objectstack/plugin-reports',
466524
export: 'ReportsServicePlugin',
467-
nameMatch: ['plugin-reports', 'ReportsServicePlugin'],
525+
identities: ['com.objectstack.service.reports', 'ReportsServicePlugin'],
468526
},
469527
approvals: {
470528
pkg: '@objectstack/plugin-approvals',
471529
export: 'ApprovalsServicePlugin',
472-
nameMatch: ['plugin-approvals', 'ApprovalsServicePlugin'],
530+
identities: ['com.objectstack.service.approvals', 'ApprovalsServicePlugin'],
473531
},
474532
settings: {
475533
pkg: '@objectstack/service-settings',
476534
export: 'SettingsServicePlugin',
477-
nameMatch: ['service-settings', 'SettingsServicePlugin'],
535+
identities: ['com.objectstack.service.settings', 'SettingsServicePlugin'],
478536
},
479537
webhooks: {
480538
pkg: '@objectstack/plugin-webhooks',
481539
export: 'WebhookOutboxPlugin',
482-
nameMatch: ['plugin-webhook-outbox', 'WebhookOutboxPlugin'],
540+
identities: ['com.objectstack.plugin-webhook-outbox', 'WebhookOutboxPlugin'],
483541
},
484542
};
485543

@@ -2328,12 +2386,11 @@ export default class Serve extends Command {
23282386
// the static registry + its token in the spec vocabulary (#3265).
23292387
const CAPABILITY_PROVIDERS = Serve.CAPABILITY_PROVIDERS;
23302388

2331-
const hasPluginMatching = (fragments: string[]) =>
2332-
plugins.some((p: any) => {
2333-
const n = String(p?.name ?? '');
2334-
const c = String(p?.constructor?.name ?? '');
2335-
return fragments.some((f) => n.includes(f) || c.includes(f));
2336-
});
2389+
// Exact identity comparison, NOT substring containment — a consumer named
2390+
// after the capability it consumes must never be mistaken for its
2391+
// provider (#7652). See Serve.providesCapability.
2392+
const hasPluginMatching = (identities: readonly string[]) =>
2393+
Serve.providesCapability(plugins, identities);
23372394

23382395
for (const cap of requires) {
23392396
const spec = CAPABILITY_PROVIDERS[cap];
@@ -2353,7 +2410,7 @@ export default class Serve extends Command {
23532410
}
23542411
continue;
23552412
}
2356-
if (hasPluginMatching(spec.nameMatch)) continue;
2413+
if (hasPluginMatching(spec.identities)) continue;
23572414

23582415
try {
23592416
const mod: any = await import(/* webpackIgnore: true */ spec.pkg);
@@ -2419,7 +2476,7 @@ export default class Serve extends Command {
24192476

24202477
if (spec.extras) {
24212478
for (const ex of spec.extras) {
2422-
if (hasPluginMatching(ex.nameMatch)) continue;
2479+
if (hasPluginMatching(ex.identities)) continue;
24232480
try {
24242481
const exMod: any = await import(/* webpackIgnore: true */ ex.pkg);
24252482
const ExCtor = exMod[ex.export];
@@ -2476,7 +2533,7 @@ export default class Serve extends Command {
24762533

24772534
if (
24782535
ExternalDatasourceServicePlugin &&
2479-
!hasPluginMatching(['service-external-datasource', 'ExternalDatasourceServicePlugin'])
2536+
!hasPluginMatching(['com.objectstack.service-external-datasource', 'ExternalDatasourceServicePlugin'])
24802537
) {
24812538
await kernel.use(new ExternalDatasourceServicePlugin());
24822539
trackPlugin('ExternalDatasourceServicePlugin');
@@ -2489,7 +2546,7 @@ export default class Serve extends Command {
24892546
const { createExternalValidationPlugin } = await import('@objectstack/runtime');
24902547
if (
24912548
createExternalValidationPlugin &&
2492-
!hasPluginMatching(['external-validation', 'ExternalValidationPlugin'])
2549+
!hasPluginMatching(['com.objectstack.external-validation', 'ExternalValidationPlugin'])
24932550
) {
24942551
await kernel.use(createExternalValidationPlugin());
24952552
trackPlugin('ExternalValidationPlugin');
@@ -2525,7 +2582,7 @@ export default class Serve extends Command {
25252582

25262583
if (
25272584
DatasourceAdminServicePlugin &&
2528-
!hasPluginMatching(['service-datasource-admin', 'DatasourceAdminServicePlugin'])
2585+
!hasPluginMatching(['com.objectstack.service-datasource-admin', 'DatasourceAdminServicePlugin'])
25292586
) {
25302587
// Lazy data-engine surface for the secret store (resolved per call
25312588
// so it works whether the engine is registered as 'data' or

0 commit comments

Comments
 (0)