feat(mcpcompat): let resource handlers return _meta

[jhrozek]

Closes #194. Part of stacklok/toolhive#6027.

Problem

wrapResourceHandler built its result as mcp.ReadResourceResult{Contents: contents}, so a resource handler had no way to set _meta and any backend resource metadata was dropped before it reached the wire. wrapPromptHandler — immediately below it — already jsonConverts the handler's whole result, which is why prompts keep _meta today and resources did not. Both ends already supported the field; only the handler signature was in the way.

Change

Adds a result-returning handler variant alongside the existing contents-returning one:

• ResourceResultHandlerFunc returning (*mcp.ReadResourceResult, error)
• ResultHandler field on ServerResource and ServerResourceTemplate (takes precedence over Handler)
• AddResourceWithResult / AddResourceTemplateWithResult
• one adapter resolving the effective handler, so all four registration paths — global resource, global template, and both per-session overlays — gain _meta support
• wrapResourceHandler forwards the handler's whole result, mirroring wrapPromptHandler

Putting ResultHandler on the two registration structs means the per-session overlay (SetSessionResources / SetSessionResourceTemplates) is covered without a new session API — that is the path ToolHive's vMCP uses.

Additive on purpose. ResourceHandlerFunc, ResourceTemplateHandlerFunc, AddResource and AddResourceTemplate are untouched; existing registrations still compile. mcp-go has no result-returning resource handler, so there is no upstream shape to diverge from.

Acceptance criteria

• A resource handler can set _meta and it appears on the resources/read wire result
• Existing ResourceHandlerFunc registrations are unchanged and still compile
• Covered for both AddResource and AddResourceTemplate

Tests

TestResourceWithResult_EndToEnd (table-driven) and TestSessionResourceWithResult_EndToEnd, both over real streamable HTTP with the mcpcompat client:

• _meta round-trips for global resource, global template, and the per-session overlay
• Handler vs ResultHandler precedence, for both ServerResource and ServerResourceTemplate
• plain contents-returning handlers still work and set no _meta

Every assertion was mutation-checked: reverting the production change makes them fail, and the "no _meta" assertions were confirmed non-tautological by force-attaching a meta key.

Review notes

Verified during review, not assumed:

• Nil-result invariant holds. json.Unmarshal("null", …) is a no-op on a struct, so the unconditionally allocated out can never be nil. This matters beyond stripPublicCacheScope's comment: go-sdk calls setDefaultCacheableValues() before its own nil check and would panic on a typed-nil.
• Contents serialization is byte-identical to before, including the nil-contents case, and cache scope is still narrowed to private.
• SetNeedsInput has no effect on this path — mcp.ReadResourceResult.resultType is unexported with no custom marshaler, so jsonConvert drops it. Documented on ResourceResultHandlerFunc.

Deliberately out of scope: a handler's _meta can shadow go-sdk's reserved io.modelcontextprotocol/serverInfo key. That hole is pre-existing and identical on wrapToolHandler and wrapPromptHandler, so it needs one shared fix rather than a resource-only patch.

task (lint + race tests) and task license-check pass.

🤖 Generated with Claude Code

[JAORMX]

Panel review — Spec / Standards / Domain axes, run independently, no cross-axis merging. Read against issue #194, downstream stacklok/toolhive#6027, parent toolhive#2640, and the merged sibling toolhive#6024 (reserved _meta stripping). Verified the go-sdk v1.7.0-pre.3 type shape directly.

Spec axis: clean. All three acceptance criteria met. All four registration paths (global AddResource/AddResourceTemplate + both per-session overlays) gain _meta support — the per-session path is the one downstream toolhive#6027 actually needs, and it's covered without a new session API. Old signatures byte-for-byte unchanged; additive as promised. No scope creep.

Standards axis: no hard violations. Purely additive; no test removal, no go.mod change, no exported-signature change. Judgement-call nits only: the new doc comments are more verbose than the file's terse one-liner convention (ResourceHandlerFunc, AddResource), and ResultHandler carries a field-level comment where sibling fields have none. Noted, not flagged inline.

Domain axis (6 reviewers, synthesized):

• 🟠 Security (Medium) — the _meta path is new forwarding surface, not neutral: pre-PR this result was hardcoded meta-less. Reserved io.modelcontextprotocol/* shadowing is real here and currently relies solely on toolhive#6024's egress strip. Recommend defense-in-depth in wrapResourceHandler, consistent with this file's own stripPublicCacheScope.
• 🟠 Correctness (Medium) — a ResultHandler returning (nil, nil) silently yields an empty {} wire result.
• 🟡 DevEx (Low) — both-fields-nil on the per-session struct-literal path fails late as a recovered panic; silent ResultHandler-over-Handler precedence is a debugging trap on that same path.
• ⚪ Docs (Info ×2) — make the template ResultHandler comment self-contained; expand the SetNeedsInput caveat with the reason, and note the type serves both resources and templates.
• Architecture / reuse / duplication / UX: clean. The resourceResultHandler adapter is the right extraction at the right time; AddResourceWithResult/AddResourceTemplateWithResult are the correct kind of boring copy; mirroring wrapPromptHandler is the right symmetry. One reuse reviewer suggested inlining the two resultHandler() accessors (net −6 lines) — rejected by the duplication reviewer (would leak the ResourceHandlerFunc cast to call sites); I side with keeping them.

Verdict: COMMENT. Nothing here blocks merge — the feature is correct, well-tested (mutation-checked, per the PR), and the spec is fully met. The two Mediums are hardening opportunities on a brand-new code path (cheap to take now, annoying to retrofit), not defects in what's shipped. The reserved-key point is the one I'd most like a maintainer's explicit decision on: strip in the shim, or document that egress owns it.

(No code changes requested; posting as COMMENT per review policy.)

[JAORMX]

[Security — Medium] New _meta forwarding surface with no reserved-key defense-in-depth

Pre-PR, wrapResourceHandler hardcoded mcp.ReadResourceResult{Contents: contents} — _meta could never reach the wire on the resource-read path. This change jsonConverts the handler's whole result, so a handler-set _meta (including reserved io.modelcontextprotocol/* keys like serverInfo) now flows through for the first time. The PR description calls this "pre-existing and identical on wrapToolHandler/wrapPromptHandler" — but those paths already forwarded _meta; this path did not. So it's strictly additive attack surface, not neutral.

The sibling consumer (stacklok/toolhive#6024, merged) strips reserved keys at vMCP egress — but that makes egress the only line of defense, and this shim already does in-body defensive rewrites elsewhere (stripPublicCacheScope, server.go:839) precisely because the shim serves per-identity and can't rely on downstream. Two options, in order of preference:

1. Defense-in-depth here (consistent with stripPublicCacheScope): strip io.modelcontextprotocol/-prefixed keys from mres.Meta.AdditionalFields before jsonConvert. Low cost, one place, protects every consumer.
2. Document the boundary explicitly: state on ResourceResultHandlerFunc that reserved-key stripping is the consumer's responsibility (egress), so the next reader doesn't assume the shim handles it.

The per-session overlay path (session.go:446,470) inherits this — session hooks now reach the wire with full _meta control too.

[JAORMX]

[Correctness/DevEx — Medium] (nil, nil) from a ResultHandler silently produces an empty wire result

If a ResourceResultHandlerFunc returns (nil, nil), jsonConvert(mres, out) marshals null, which unmarshals into an empty gosdk.ReadResourceResult{} — the client receives {} with no contents, no error, and no signal anything went wrong. The old contents-returning path couldn't produce this (a nil []ResourceContents still marshals a non-nil result). A one-line guard makes the failure loud instead of silent:

mres, err := h(ctx, mreq)
if err != nil {
    return nil, err
}
if mres == nil {
    return nil, fmt.Errorf("resource handler %q returned a nil result", req.Params.URI)
}

Worth a guard here (the shared chokepoint all four registration paths flow through) rather than documenting the invariant and hoping every handler honours it.

[JAORMX]

[DevEx — Low] Both-fields-nil registration fails late and opaquely on the per-session path

When a ServerResource/ServerResourceTemplate is built with neither Handler nor ResultHandler set, this adapter returns a closure that calls nil(ctx, req) → nil-pointer dereference → caught by the recover() in wrapResourceHandler and surfaced as "panic recovered in resource handler %q: runtime error: invalid memory address...". The AddResource*/AddResourceWithResult methods always populate one field, so this is only reachable via the per-session overlay (SetSessionResources/SetSessionResourceTemplates) where callers construct the struct literally — which is exactly the path ToolHive's vMCP uses (the downstream consumer this PR is for). A nil-guard in resourceResultHandler (return an error result or a clear sentinel) would surface the mistake at the point of misconfiguration rather than as a recovered panic mid-request. Not a blocker — but the error a developer gets today points at the wrong layer.

[JAORMX]

[API design — Low] Silent precedence is a debugging trap on the struct-literal path

When both Handler and ResultHandler are set, ResultHandler silently wins and Handler is dead code. The field comments document the rule, but nothing enforces or signals it — a contributor setting both fields in a SetSessionResources call gets no compile error, no log, just a handler that never fires. AddResourceWithResult avoids the ambiguity by populating only ResultHandler, but the per-session struct-literal path (again, the vMCP consumer path) doesn't. Consider a slog.Warn in resourceResultHandler when both are non-nil — negligible cost, makes the footgun loud without changing the public API. (Architect panel's top finding; security/dev-ex axes independently flagged the adjacent nil cases, so this whole adapter is worth one hardening pass.)

[JAORMX]

[Docs — Info] Make ServerResourceTemplate.ResultHandler self-contained

This comment forwards to ServerResource.ResultHandler for the why, so a developer reading ServerResourceTemplate in isolation has to navigate away to learn the purpose. ServerResource.ResultHandler (line 111-113) says "is the only way to return _meta on a resource read" — adding the parallel clause here ("… and is the only way to return _meta on a resource template read") makes both structs self-contained. One line.

[JAORMX]

[Docs — Info] Two small additions to this doc comment

1. The SetNeedsInput caveat is discoverable but cryptic — a consumer who reads "has no effect" and tries anyway gets a silent no-op. Adding the why in one clause makes it actionable: "…has no effect on this path because the underlying resultType field is internal to go-sdk and excluded from JSON marshaling."

2. Worth stating explicitly that this type serves both resources and resource templates (ServerResource.ResultHandler and ServerResourceTemplate.ResultHandler share it, with the template bridging via a ResourceHandlerFunc cast at server.go:1070). The type asymmetry between Handler (ResourceTemplateHandlerFunc) and ResultHandler (shared type) looks accidental without that note.