Uh oh!
There was an error while loading. Please reload this page.
fix(mcp): read skill prompts from the protocol's merged metadata listing - #8724
Merged
Merged
Conversation
The MCP prompt bridge read `IMetadataService.list('skill')` — one layer
below where the `sys_metadata` overlay merge happens — so a runtime
`PUT /api/v1/meta/skill/<name>` returned 200 and never reached MCP
prompts, while `GET /api/v1/meta/skill` served the overridden row from
the protocol's `getMetaItems`.
The bridge now takes an optional merged read (the protocol service,
wired by the plugin assembly that can see both services) and takes its
items from there, keeping #6504's completeness verdict by asking
`listDiagnosed` for it alongside — `getMetaItems` cannot express that
verdict, so taking its answer alone would have spent the contract.
A merged read that throws does NOT fall back to the un-merged listing:
that would answer registry rows in the shape of merged ones at exactly
the moment an overlay is most likely to be missed.
Also marks the trap at its source: `IMetadataService.list()` now states
that it performs no overlay merging (comment only).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53TnThe case asserted that `list()` was never called, which is true in both directions — `diagnosedList` prefers `listDiagnosed` whenever the service has it, so the un-merged path never touches `list` either. Measured 5 red / 3 green against a predicted 6 / 2; the third green was this defective assertion, not a third invariant. Now asserts provenance directly: the service holds a projectable skill, the merged read holds none, and nothing is bridged. Re-measured 6 / 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 111 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
This was referenced Aug 14, 2026
qq9340100
marked this pull request as ready for review
August 14, 2026 16:54
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #8328
Implements the maintainer ruling recorded on #8328 (option 3, 2026-08-13 ~23:50Z): point the MCP prompt bridge's skill read at the protocol layer's merged read, mark the trap at its source, and archive the contract question unscheduled.
Part of, notFixes— merging this does not close the card. The card's own three-step reproduction runs through a second skill read that lives outside this PR's declared file surface. Measured, not assumed; see "The half this PR does not fix" below.The defect, measured on a booted showcase
origin/main@3508678, real showcase,PUT /api/v1/meta/skill/repro_skillwith{active:true}→ 200:GET /api/v1/meta/skill(protocolgetMetaItems)active:trueprompts/list{"prompts":[]}Re-queried minutes later, well past the
DatabaseLoader60s TTL and the list cache: still[]. Structural, not a cache artifact.The cause is the layer: the bridge read
IMetadataService.list('skill'), which answers from the registry and its loaders and performs nosys_metadataoverlay merge. That merge lives one layer up, in the protocol'sgetMetaItems.What changed
bridgePromptstakes an optional merged read (McpMergedMetadataRead, a duck-typedgetMetaItemsseam) and takes its skill items from it.plugin.tsresolves theprotocolservice and passes it — the assembly is the only place that can see both services, since the runtime is handed its collaborators and holds no service registry.IMetadataService.list() presents a known-partial answer as a complete one — the #5840 shape on the plural read #6504's completeness verdict is preserved, and this is the part that needed care.
getMetaItemscannot express it: it swallows a MetadataService read failure into its owncatchand returns a merged list either way. So items come from the merged read and the verdict is asked oflistDiagnosedalongside it — the same composition this file already uses forobjectstack://objects. TakinggetMetaItems' answer alone would have silently spent the contract, sending a known-partial prompt surface back to presenting as complete.No fallback to the un-merged listing when the merged read throws. That would answer registry rows in the shape of merged ones — this exact defect, restored silently at the moment an overlay is most likely to be the thing being missed.
getMetaItemsalready answers registry-only for the one benign case (sys_metadatanot provisioned yet).Ruling item 2 — the trap is marked at its source.
IMetadataService.list()'s TSDoc now states that the read performs no overlay merging, namesgetMetaItemsas the merged read, and says why there is no local symptom. Comment-only, verified mechanically: the non-comment diff of that file is empty.Ruling item 3 — the contract question is archived as Should the sys_metadata overlay merge move down into MetadataService.list(), so every consumer gets merged rows? #8722 (
finding+domain:metadata, unassigned, back-linked). Deduped against open issues by keyword and file path first; no existing card covers it.Ruling item 4 is respected: the override merge is not pushed down into
MetadataService. Nothing outsidepackages/mcpchanged except the authorized comment.Live before/after — same persistent DB, same overlay row
Boot,
PUTthe skill, restart, read the bridge's own boot line:This also settles the dispatch's flagged assumption — that
getMetaItemsmight not be reachable from the bridge's construction context. It is reachable.Protocol service registered (MetadataProtocolPlugin)lands well before the MCP plugin'sstart(), and the "protocol service not available" fallback line was never emitted (0 occurrences). No wiring beyond what the assembly already had.The half this PR does not fix
There are two skill reads behind MCP prompts, not one:
packages/mcp/src/mcp-server-runtime.ts→bridgePrompts— the long-lived (stdio) server. Fixed here.packages/runtime/src/domains/mcp.ts→buildMcpBridge.listSkills— the HTTP surface at/api/v1/mcp, built per request by the runtime. Readsmeta.list('skill')directly, with no diagnosed wrapper at all. Not fixed here — outside the declared file surface.The card's reproduction and the
pnpm devbanner both point at the HTTP endpoint, so after this PR that reproduction still returns{"prompts":[]}— verified against the running server with the fix in place. The ruling's decision (read from the merged layer rather than pushing the merge down) is unaffected; only its scope estimate — "entirely insidepackages/mcp" — turned out to cover half the surface. The remaining half applies the same decision at a second call site in a different lane and is left for the PM to route.Tests
New:
packages/mcp/src/mcp-server-runtime.merged-skill-read.test.ts— 8 cases pinning the layer the bridge reads from, that the service's own listing contributes no items, the enveloped and bare-array shapes, both directions of theactiveflip, the no-fallback rule, and that the #6504 degraded verdict still reaches the operator.Reverse verification, direction predicted before running: 6 red / 2 green. Measured 5 red / 3 green — the prediction was wrong. The extra green was a defective assertion, not a third invariant: the case asserted
list()was never called, which is true in both directions becausediagnosedListpreferslistDiagnosedwhenever the service has it. Rewritten to assert provenance directly (service holds a projectable skill, merged read holds none, nothing is bridged); re-measured 6 red / 2 green, with the two green being the two predicted invariants. Both numbers and the reason are recorded in the test file header.Verification
All at final head
d23577894, after the last commit:pnpm --filter @objectstack/mcp test— 18 files / 193 tests passedpnpm --filter @objectstack/mcp --filter @objectstack/spec typecheck— passpnpm check:type-check-debt(--re-measure, 33 ledger entries, 585s) — pass, "none above its recorded number"check:nul-bytes,check:cross-package-test-inputs(+ the direct script),check:changeset-gate-self-tests,check:merge-driver,check:spec-parsed-alias,check:type-source-resolution,check-adr-0087-registration.mjs,check:query-options-erasure,check:type-check-coverage— all passThe gate set was re-derived from the actual diff rather than taken from the dispatch list: because ruling 2's comment lands in
packages/spec, seven gate families applied that the dispatch prompt did not name (the spec-scoped ones plus the new-test-file conventions). They are included above.Generated by Claude Code