diff --git a/.changeset/mcp-prompt-bridge-merged-skill-read.md b/.changeset/mcp-prompt-bridge-merged-skill-read.md new file mode 100644 index 0000000000..11d587d1cc --- /dev/null +++ b/.changeset/mcp-prompt-bridge-merged-skill-read.md @@ -0,0 +1,14 @@ +--- +"@objectstack/mcp": patch +--- + +fix(mcp): the skill prompt bridge reads the protocol's merged metadata listing, so a runtime `PUT /api/v1/meta/skill/` reaches MCP prompts (#8328) + +The bridge read `IMetadataService.list('skill')` — one layer below where the +`sys_metadata` overlay merge happens — so an override returned 200 and never +reached the prompt surface while `GET /api/v1/meta/skill` served it. The +long-lived (stdio) server's bridge now takes its items from the protocol's +`getMetaItems` when the host can supply it, and keeps the #6504 completeness +verdict by asking `listDiagnosed` for it alongside. A host assembled without the +metadata protocol reads exactly as before, and a merged read that throws does not +fall back to the un-merged listing. diff --git a/packages/mcp/src/mcp-server-runtime.merged-skill-read.test.ts b/packages/mcp/src/mcp-server-runtime.merged-skill-read.test.ts new file mode 100644 index 0000000000..663d3f2f0d --- /dev/null +++ b/packages/mcp/src/mcp-server-runtime.merged-skill-read.test.ts @@ -0,0 +1,267 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8328] The skill prompt bridge reads the MERGED metadata listing, so a + * runtime meta override reaches MCP prompts. + * + * --------------------------------------------------------------------------- + * The defect + * --------------------------------------------------------------------------- + * `PUT /api/v1/meta/skill/` with `{active:true}` returned 200 and the + * flip never reached the prompt surface. The two surfaces read different + * layers: the meta HTTP list goes through the protocol's `getMetaItems`, which + * merges the `sys_metadata` overlay over the registry / MetadataService + * baselines, while this bridge read `IMetadataService.list('skill')` — one + * layer BELOW where any overlay merging happens. Same skill name, two answers. + * + * Measured on a booted showcase before the fix: `GET /api/v1/meta/skill` + * served the overridden row and `[MCP] Bridged 0 skill prompts` was logged at + * the same boot. The `active` flip is what makes the count binary — a row with + * `active:false` is not projected at all (`projectSkillPrompt`) — so the + * override's arrival is visible as 0 → 1 rather than as a body diff. + * + * --------------------------------------------------------------------------- + * What this file pins, and what it deliberately does NOT + * --------------------------------------------------------------------------- + * It pins the LAYER the bridge reads from, and that #6504's completeness + * verdict survives the layer change. It does not re-pin the projection rules + * (`skill-prompts.test.ts` owns those) or the overlay merge itself + * (`packages/metadata-protocol` owns that — `getMetaItems` is a double here). + * + * ⚠️ Scope, stated so a reader does not over-read a green file: this covers the + * LONG-LIVED server's bridge, which is the stdio transport's prompt surface and + * the half of #8328 that lives in this package. The HTTP surface at + * `/api/v1/mcp` builds its bridge in `packages/runtime` + * (`domains/mcp.ts` → `buildMcpBridge`), whose `listSkills` is a separate read + * that this file cannot reach and that is NOT fixed by this change. + * + * --------------------------------------------------------------------------- + * Reverse verification, direction predicted BEFORE running + * --------------------------------------------------------------------------- + * Ordinary red, on the consumer. The reversion is behavioural: `listSkills` + * goes back to `diagnosedList(metadataService, 'skill')`, ignoring the merged + * read. Predicted, written down before running: **6 red / 2 green**. + * + * The two predicted GREEN are invariant pins, green in both directions on + * purpose: + * - *"no merged read: the bridge reads exactly as it did before"* — that case + * IS the pre-fix behaviour, so it must not move; + * - *"a degraded verdict still reaches the operator"* — the warn came from + * `listDiagnosed` before this change and still does. It would go red if a + * future change spent the #6504 contract while switching layers, which is + * the specific regression this fix had to avoid. + * + * MEASURED: **5 red / 3 green** — the prediction was wrong, and the third green + * was a defective assertion rather than a third invariant. The case then read + * *"the un-merged `list()` is not consulted"*, which is true in BOTH directions: + * `diagnosedList` prefers `listDiagnosed` whenever the service has it, so the + * un-merged path never touches `list` either. It was rewritten to assert + * PROVENANCE (the service holds a projectable skill, the merged read holds + * none, and nothing is bridged), which discriminates. Re-measured after that + * rewrite: **6 red / 2 green**, as recorded in the PR body. + * + * The doubles declare metadata reads only — no engine write verb — so there is + * no `delete`/`update` dispatch for `check:engine-double-contract` to scan. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { IMetadataService, Logger } from '@objectstack/spec/contracts'; +import { MCPServerRuntime } from './mcp-server-runtime.js'; +import type { McpMergedMetadataRead } from './mcp-server-runtime.js'; + +type AnyRecord = Record; + +/** See the sibling outage suite: typed because the TEST_DEBT ratchet reads it. */ +type MockLogger = Logger & { + debug: ReturnType; + info: ReturnType; + warn: ReturnType; + error: ReturnType; +}; + +const makeLogger = (): MockLogger => + ({ debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }) as unknown as MockLogger; + +const infoLines = (logger: MockLogger): string => + logger.info.mock.calls.map((c: unknown[]) => String(c[0])).join('\n'); +const warnLines = (logger: MockLogger): string => + logger.warn.mock.calls.map((c: unknown[]) => String(c[0])).join('\n'); + +const LOADER_FAILURE = 'database: connect ECONNREFUSED 10.0.0.5:5432'; + +/** + * The packaged skill as the registry/loader layer holds it: authored inactive, + * so it projects to NO prompt. + */ +const PACKAGED_SKILL = { + name: 'case_management', + label: 'Case Management', + instructions: 'Handle the support case lifecycle.', + active: false, +}; + +/** + * The same skill as the protocol's merged read answers AFTER a runtime + * `PUT /api/v1/meta/skill/case_management` with `{active:true}` — the overlay + * row won its slot, so the row is now projectable. + */ +const OVERRIDDEN_SKILL = { ...PACKAGED_SKILL, active: true }; + +/** + * Build a metadata-service double. Every required member throws, so a path that + * reaches one this change should not touch fails loudly rather than resolving + * an empty array and looking like the very absence under test. + */ +function makeService(overrides: AnyRecord): IMetadataService { + const unexpected = (member: string) => async (): Promise => { + throw new Error(`double: ${member}() should not be called by this surface`); + }; + return { + register: unexpected('register'), + get: unexpected('get'), + list: unexpected('list'), + unregister: unexpected('unregister'), + exists: unexpected('exists'), + listNames: unexpected('listNames'), + getObject: unexpected('getObject'), + listObjects: unexpected('listObjects'), + ...overrides, + } as unknown as IMetadataService; +} + +/** A healthy service holding the packaged (inactive) row. */ +const packagedService = (items: unknown[] = [PACKAGED_SKILL]) => + makeService({ + list: vi.fn(async () => items), + listDiagnosed: vi.fn(async () => ({ items, degraded: false, errors: [] })), + }); + +/** A service whose loader set is short — the #6504 verdict is `degraded`. */ +const degradedService = (items: unknown[] = [PACKAGED_SKILL]) => + makeService({ + list: vi.fn(async () => items), + listDiagnosed: vi.fn(async () => ({ items, degraded: true, errors: [LOADER_FAILURE] })), + }); + +/** A service predating #6504: no `listDiagnosed` to probe. */ +const undiagnosableService = (items: unknown[] = [PACKAGED_SKILL]) => + makeService({ list: vi.fn(async () => items) }); + +/** The protocol's merged listing, in its native `{ type, items }` envelope. */ +const mergedRead = (items: unknown[]): McpMergedMetadataRead => ({ + getMetaItems: vi.fn(async ({ type }: { type: string }) => ({ type, items })), +}); + +const bridge = async ( + service: IMetadataService, + logger: MockLogger, + merged?: McpMergedMetadataRead, +): Promise => { + const runtime = new MCPServerRuntime({ name: 'merged-skill-read', version: '0.0.0', logger }); + await runtime.bridgePrompts(service, merged); +}; + +describe('#8328 — the skill read goes through the protocol\'s merged listing', () => { + it('THE DEFECT: a runtime override that only the merged read can see reaches the prompt surface', async () => { + const logger = makeLogger(); + // The registry/loader layer still holds `active:false`; only the merged + // read carries the overlay's `active:true`. Before this fix the bridge read + // the former and registered nothing. + await bridge(packagedService(), logger, mergedRead([OVERRIDDEN_SKILL])); + + expect(infoLines(logger)).toMatch(/Bridged 1 skill prompts/); + }); + + it('the metadata service\'s own listing contributes NO items', async () => { + const logger = makeLogger(); + // Provenance, asserted the only way that discriminates: the service holds a + // perfectly projectable skill and the merged read holds none. If any item + // still reached the surface it came from the layer this fix stopped + // reading. + // + // An earlier draft asserted `list()` was never called, which is TRUE in + // both directions and therefore measures nothing: `diagnosedList` prefers + // `listDiagnosed` when the service has it, so the un-merged path reads that + // member instead. Kept as a secondary assertion, not the discriminator. + const service = packagedService([{ ...PACKAGED_SKILL, name: 'stale_skill', active: true }]); + await bridge(service, logger, mergedRead([])); + + expect(infoLines(logger)).toMatch(/Bridged 0 skill prompts/); + expect(service.list).not.toHaveBeenCalled(); + }); + + it('asks the merged read for the `skill` type specifically', async () => { + const logger = makeLogger(); + const merged = mergedRead([OVERRIDDEN_SKILL]); + await bridge(packagedService(), logger, merged); + + expect(merged.getMetaItems).toHaveBeenCalledWith({ type: 'skill' }); + }); + + it('accepts a bare array from a host whose merged read is not enveloped', async () => { + const logger = makeLogger(); + const merged = { getMetaItems: vi.fn(async () => [OVERRIDDEN_SKILL]) }; + await bridge(packagedService(), logger, merged); + + expect(infoLines(logger)).toMatch(/Bridged 1 skill prompts/); + }); + + it('an override that DEACTIVATES a packaged skill retires its prompt', async () => { + const logger = makeLogger(); + // The mirror of the repro, and the case that proves the merged read is the + // authority rather than merely an additional source: the registry says + // active, the overlay says inactive, and the surface follows the overlay. + const active = { ...PACKAGED_SKILL, active: true }; + await bridge( + packagedService([active]), + logger, + mergedRead([{ ...active, active: false }]), + ); + + expect(infoLines(logger)).toMatch(/Bridged 0 skill prompts/); + }); + + it('a merged read that FAILS never falls back to the un-merged listing', async () => { + const logger = makeLogger(); + const service = packagedService(); + const merged = { + getMetaItems: vi.fn(async () => { + throw new Error('sys_metadata unreadable'); + }), + }; + await bridge(service, logger, merged); + + // Falling back would answer with registry rows in the shape of merged ones + // — this defect, restored silently at exactly the moment an overlay is most + // likely to be the thing being missed. + expect(service.list).not.toHaveBeenCalled(); + expect(warnLines(logger)).toMatch(/Could not read skill metadata/); + expect(infoLines(logger)).not.toMatch(/Bridged \d+ skill prompts/); + }); + + it('#6504 SURVIVES: a degraded verdict still reaches the operator through the merged read', async () => { + const logger = makeLogger(); + // `getMetaItems` cannot express this — it swallows a MetadataService read + // failure into its own catch — so the verdict is asked of `listDiagnosed` + // alongside it. Losing this was the live risk of the layer change. + await bridge(degradedService(), logger, mergedRead([OVERRIDDEN_SKILL])); + + const lines = warnLines(logger); + expect(lines).toMatch(/INCOMPLETE/); + expect(lines).toMatch(/missing, NOT undeclared/); + expect(logger.warn.mock.calls[0]![1].errors).toEqual([LOADER_FAILURE]); + }); + + it('no merged read: the bridge reads exactly as it did before #8328', async () => { + const logger = makeLogger(); + // A host assembled without the metadata protocol has no merged read to + // give. `undefined` means "this host cannot merge", never "merging was + // skipped", so the pre-#8328 read is the honest answer rather than a + // regression — and a service predating #6504 keeps working too. + const service = undiagnosableService([{ ...PACKAGED_SKILL, active: true }]); + await bridge(service, logger, undefined); + + expect(service.list).toHaveBeenCalledWith('skill'); + expect(infoLines(logger)).toMatch(/Bridged 1 skill prompts/); + }); +}); diff --git a/packages/mcp/src/mcp-server-runtime.ts b/packages/mcp/src/mcp-server-runtime.ts index 3e048f77af..85b2f85b03 100644 --- a/packages/mcp/src/mcp-server-runtime.ts +++ b/packages/mcp/src/mcp-server-runtime.ts @@ -221,6 +221,83 @@ async function diagnoseListRead( }; } +/** + * [#8328] The protocol layer's overlay-aware merged read. + * + * `IMetadataService.list()` is the registry/loader listing and applies **no** + * `sys_metadata` overlay merge — a runtime `PUT /api/v1/meta//` + * lands in that store and never reaches it. The merge lives one layer up, in + * the protocol's `getMetaItems`, which reads the overlay rows and resolves them + * per `(slot, package)` over the registry and MetadataService baselines. + * + * Structurally optional, and NOT the optionality `listDiagnosed` has: a host + * that assembles this runtime without the metadata protocol has no merged read + * to offer at all, so the seam is absent rather than degraded. `undefined` here + * therefore means "this host cannot merge", never "merging was skipped". + */ +export interface McpMergedMetadataRead { + /** The protocol's merged listing for one metadata type. */ + getMetaItems(request: { type: string }): Promise; +} + +/** + * Coerce a `getMetaItems` answer into its items array. + * + * The protocol answers `{ type, items }`, but the shape is read defensively for + * the reason `packages/rest` reads it defensively at every one of its own call + * sites: this is a duck-typed seam across a package boundary, and a host may + * hand back the bare array. + */ +function metaItemsArray(answer: unknown): unknown[] { + if (Array.isArray(answer)) return answer; + const items = (answer as { items?: unknown } | null | undefined)?.items; + return Array.isArray(items) ? items : []; +} + +/** + * [#8328] List one metadata type through the **merged** read when this host has + * one, keeping #6504's completeness verdict on top of it. + * + * The defect this closes: the skill prompt surface read `list()` one layer + * below where any overlay merging happens, so a runtime meta PUT returned 200 + * and the flip never reached MCP prompts, while `GET /api/v1/meta/skill` served + * it from the merged read. Two surfaces, one skill name, two answers. + * + * The composition is the point, and it is the one this file already uses for + * `objectstack://objects`: **items** come from the merged read, and the + * **verdict** from {@link diagnoseListRead}, which asks `listDiagnosed` the + * question `getMetaItems` cannot answer. `getMetaItems` swallows a + * MetadataService read failure into its own `catch` and reports a merged list + * either way, so taking its answer alone would have silently spent the + * degraded/errors contract #6504 installed — a known-partial prompt surface + * would go back to presenting as a complete one. Asking the metadata service + * directly keeps that verdict addressed to the same set the items came from: + * the merged list is the registry/overlay layers ON TOP of exactly the + * MetadataService listing whose completeness is being judged, so a loader that + * could not be read makes both short together. + * + * ⛔ No fallback to the un-merged `list()` when the merged read THROWS. That + * would answer with registry rows in the shape of merged ones — the very defect + * above, restored silently at the moment the overlay store is unreadable, which + * is exactly when an overlay is most likely to be the thing being missed. + * `getMetaItems` already answers registry-only for the one benign case (the + * `sys_metadata` table not being provisioned yet); anything it raises past that + * means overlay rows may exist and were not seen, and the caller's own handling + * — a warn and no skill prompts — is the honest report. + */ +async function mergedDiagnosedList( + metadataService: IMetadataService, + mergedRead: McpMergedMetadataRead | undefined, + type: string, +): Promise { + if (!mergedRead || typeof mergedRead.getMetaItems !== 'function') { + return diagnosedList(metadataService, type); + } + const items = metaItemsArray(await mergedRead.getMetaItems({ type })); + const verdict = await diagnoseListRead(metadataService, type); + return { items, degraded: verdict.degraded, errors: verdict.errors }; +} + /** * [#6055] Read one metadata item, keeping the ADR-0110 D3 verdict instead of * flattening an outage into the same `undefined` a never-declared name @@ -960,8 +1037,27 @@ export class MCPServerRuntime { * prompt's **body** is re-read from metadata at `prompts/get` time, so an * edited skill serves fresh text without a restart. The HTTP transport builds * its server per request and is live on both — see {@link handleHttpRequest}. + * + * [#8328] `mergedRead` is the protocol layer's overlay-aware listing (see + * {@link McpMergedMetadataRead}). When the host can supply it, the skill read + * goes through it so a runtime `PUT /api/v1/meta/skill/` reaches this + * surface; without it the read is the pre-#8328 one, unchanged. It is a + * parameter rather than something resolved in here because this runtime is + * handed its collaborators and holds no service registry of its own — the + * assembly that knows both services wires them together (`plugin.ts`). + * + * ⚠️ This bridges the **long-lived** server only — the one the stdio + * transport serves. The HTTP surface at `/api/v1/mcp` builds a fresh server + * per request from a bridge the RUNTIME supplies + * (`packages/runtime/src/domains/mcp.ts` → `buildMcpBridge`), and its + * `listSkills` is a separate read that this parameter cannot reach. Both + * surfaces have to be pointed at the merged read to close #8328; this one is + * the half that lives in this package. */ - async bridgePrompts(metadataService: IMetadataService): Promise { + async bridgePrompts( + metadataService: IMetadataService, + mergedRead?: McpMergedMetadataRead, + ): Promise { const logger = this.config.logger; // Register a dynamic prompt that loads agents at call time @@ -1012,7 +1108,11 @@ export class MCPServerRuntime { // is also the per-call re-read behind each registered prompt's body, so // this is deliberately last-read-wins rather than boot-only: the // snapshot check below runs immediately after its own call. - const read = await diagnosedList(metadataService, 'skill'); + // + // [#8328] Through the merged read when this host has one — the whole + // point of the re-read above is that an edited skill serves fresh text, + // and a runtime meta PUT is the edit that never arrived. + const read = await mergedDiagnosedList(metadataService, mergedRead, 'skill'); skillListVerdict = { degraded: read.degraded, errors: read.errors }; return read.items; }, diff --git a/packages/mcp/src/plugin.ts b/packages/mcp/src/plugin.ts index 871b79724a..dd0735f18d 100644 --- a/packages/mcp/src/plugin.ts +++ b/packages/mcp/src/plugin.ts @@ -6,7 +6,7 @@ import { readEnvWithDeprecation, isMcpServerEnabled, resolveMcpStdioAutoStart } import type { ExecutionContext } from '@objectstack/spec/kernel'; import type { IAIService, IDataEngine, IMetadataService } from '@objectstack/spec/contracts'; import { MCPServerRuntime } from './mcp-server-runtime.js'; -import type { MCPServerRuntimeConfig } from './mcp-server-runtime.js'; +import type { MCPServerRuntimeConfig, McpMergedMetadataRead } from './mcp-server-runtime.js'; import type { ToolRegistry } from './types.js'; import { createStdioDataBridge, enforceApiExposure, GATED_ACTIONS } from './stdio-data-bridge.js'; import type { McpDataBridge } from './mcp-http-tools.js'; @@ -153,6 +153,35 @@ export class MCPServerPlugin implements Plugin { ctx.logger.debug('[MCP] Metadata service not available, skipping resource bridging'); } + // ── Merged (overlay-aware) metadata read for the prompt bridge (#8328) ── + // The protocol service is the layer that merges `sys_metadata` overlay rows + // over the registry / MetadataService baselines; the metadata service alone + // is one layer BELOW that merge. Resolved here, next to the metadata + // service, because this assembly is the only place that can see both — the + // runtime is handed its collaborators and keeps no service registry. + // + // Absent is a supported state, not a failure: a host assembled without the + // metadata protocol has no merged read to give, and the bridge then reads + // exactly as it did before #8328. Same duck-typed `getMetaItems` probe the + // REST layer applies to this service, for the same reason — the shim is + // registered by two different mounts (`MetadataProtocolPlugin` and + // ObjectQLPlugin's built-in `registerProtocol` mode). + let mergedRead: McpMergedMetadataRead | undefined; + try { + const protocol = ctx.getService('protocol'); + if (protocol && typeof protocol.getMetaItems === 'function') { + mergedRead = protocol; + } else { + ctx.logger.debug( + '[MCP] Protocol service has no getMetaItems — skill prompts read the un-merged metadata listing (runtime meta overrides will not be reflected)', + ); + } + } catch { + ctx.logger.debug( + '[MCP] Protocol service not available — skill prompts read the un-merged metadata listing (runtime meta overrides will not be reflected)', + ); + } + // ── stdio auto-start decision (opt-in, its OWN switch) ── // Deliberately stricter than the HTTP-surface default (`isMcpServerEnabled`, // default-on): start() attaches a long-lived transport claiming the @@ -281,7 +310,9 @@ export class MCPServerPlugin implements Plugin { // Awaited: the prompt bridge reads `skill` metadata to project each // skill's instructions onto an MCP prompt (#3905), so the surface must be // complete before the transport attaches below. - await this.runtime.bridgePrompts(metadataService); + // [#8328] `mergedRead` points the skill read at the overlay-aware layer + // so a runtime `PUT /api/v1/meta/skill/` reaches this surface. + await this.runtime.bridgePrompts(metadataService, mergedRead); } // [#8034] BEFORE `start()`, with the resources and prompts: registering a diff --git a/packages/spec/src/contracts/metadata-service.ts b/packages/spec/src/contracts/metadata-service.ts index 82928b3e8e..0ed1dd45e4 100644 --- a/packages/spec/src/contracts/metadata-service.ts +++ b/packages/spec/src/contracts/metadata-service.ts @@ -347,6 +347,30 @@ export interface IMetadataService { * especially before restating the array's LENGTH as a fact about the * environment (#6504). * + * ⚠️ **THIS READ PERFORMS NO OVERLAY MERGING (#8328).** It answers from the + * registry and this service's own loaders. It does **not** apply the + * `sys_metadata` customization overlay, which is merged one layer ABOVE + * this contract by the metadata protocol's `getMetaItems` /`getMetaItem` + * (per `(slot, package)`, org-scoped rows over env-wide ones). So after a + * runtime `PUT /api/v1/meta//` returns 200, this member keeps + * answering with the packaged/registered row while + * `GET /api/v1/meta/` serves the overridden one — same name, two + * answers, no local symptom. + * + * The trap is that nothing about the call site shows it: the type is the + * same, the shape is the same, and only the CONTENT is stale, and only when + * an override happens to exist. A consumer that must reflect runtime + * overrides has to read the protocol's merged listing instead; `list` is + * the right read for the registry/loader set itself, and for consumers that + * genuinely want the authored baseline. Measured on the MCP prompt bridge, + * which read `list('skill')` and served prompts an admin had already + * overridden through the meta API. + * + * Whether the merge BELONGS down here — so every `list()` consumer gets it + * rather than each one remembering to climb a layer — is a live contract + * question, deliberately left open rather than answered by this note; it is + * archived unscheduled and re-grades on real demand. + * * @param type - Metadata type * @returns Array of metadata definitions */