Repository navigation
Conversation
A single tools/list item the compat types cannot decode, such as a tool whose top-level inputSchema "type" is a JSON Schema type array, failed the whole page. The backend then listed no tools at all, so nothing downstream could use or manage its other tools. Decode list items one at a time and skip an item that fails, logging a warning that names it. A list field that is not an array still fails the page. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6772 +/- ##
==========================================
+ Coverage 79.43% 79.47% +0.03%
==========================================
Files 808 808
Lines 82060 82082 +22
==========================================
+ Hits 65186 65234 +48
+ Misses 16869 16843 -26
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The skip warning had no backend identity, unlike the neighbouring list warnings, so with several backends it was hard to trace which one served the malformed item. Thread the backend name into modernListAll. Also skip JSON null items, which previously decoded into a zero-value item and were kept, and cover resources, prompts and null items in a single table-driven test. The undecodable fixture is now a wrongly typed field rather than a schema type array, so it stays undecodable once toolhive-core accepts type arrays. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
github-actions
Bot
added
size/S
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Thanks, this is a clear improvement. One bad item taking out a whole backend was a nasty failure mode, and dropping just that item with a warning is pretty much what the 2026-07-28 spec asks for with bad x-mcp-header tool definitions: exclude the tool from tools/list and log its name and the reason. Nothing here blocks merging. I left a few inline notes.
Two things that aren't tied to a line:
The Legacy path still fails the whole list. queryTools and friends go through the compat client's ListTools, which runs the result through convertResult (toolhive-core mcpcompat/client/client.go:421). That marshals and then unmarshals the whole ListToolsResult in one go, so the same backend shows all but one of its tools on Modern and none on Legacy. You already said in the description that this PR only covers Modern, which is fine. Could you open a follow-up issue so it doesn't get lost? A fallback decode on Tool in toolhive-core would probably fix both paths in one place.
The test plan in the description still names TestModernEnumerate_SkipsUndecodableTool and TestModernEnumerate_MalformedItemsFieldFails, and still mentions the type-array fixture. The second commit replaced those with TestModernEnumerate_UndecodableItems, so it's worth updating the description.
| // Items are decoded one at a time. An item the compat types cannot decode (for | ||
| // example a tool whose top-level schema "type" is a JSON Schema type array) is | ||
| // skipped with a warning naming it, so one malformed item does not hide every | ||
| // other item the backend advertises. A JSON null item carries nothing to |
There was a problem hiding this comment.
Small wording point. For inputSchema, a type array really is malformed, since the spec requires type: "object" at the root. outputSchema is different though. In 2026-07-28 it can be any JSON Schema 2020-12, so {"type": ["object", "null"]} there is valid. ToolOutputSchema is the same ToolArgumentsSchema with Type string (toolhive-core mcp/tools.go:408,427), so a valid tool like that gets skipped here.
Skipping it is still much better than failing the whole backend, so I'm fine with the behavior for now. I'd just avoid wording here and in the description that suggests every skipped item breaks the spec, because otherwise someone reading the warning will go chase the backend. The toolhive-core follow-up could target outputSchema specifically, maybe by falling back to RawOutputSchema when the typed decode fails.
| func itemIdentifier(raw json.RawMessage) string { | ||
| var id struct { | ||
| Name string `json:"name"` | ||
| URI string `json:"uri"` | ||
| } | ||
| if err := json.Unmarshal(raw, &id); err != nil { | ||
| return "" | ||
| } | ||
| if id.Name != "" { | ||
| return id.Name | ||
| } | ||
| return id.URI |
There was a problem hiding this comment.
Because name and uri decode into one struct, a type error on either one makes this return "". Your resource fixture {"name": 123, "uri": "file:///bad"} hits exactly that: the warning has no identifier even though the uri was fine. Decoding into map[string]json.RawMessage and trying each key separately would fix it. Not a big deal, since the index is logged too.
A related nit: this logs a backend-supplied uri at WARN. I don't think that's a real problem. Resource URIs are sent to every client anyway, and the aggregator and handler_factory already log them at WARN. If you'd rather be conservative, logging just name and index works too.
| }, | ||
| }, | ||
| { | ||
| name: "tools field that is not an array fails the enumeration", |
There was a problem hiding this comment.
Nice table. A few cases might be worth adding while you're here. One is a bad item in resourceTemplates, which goes through the same path but isn't exercised. Another is a page with a bad item plus a nextCursor, to show we still follow the cursor and keep page 2. The last is a page where every item is bad, which should give an empty list and no error.
This branch has not been deployed
2 participants
Summary
tools/listitem that the compat types cannot decode fails the whole page. The Modern enumeration path decodes thetoolsarray in onejson.Unmarshal, so one tool whose top-level schematypeis a JSON Schema type array (valid JSON Schema, for example["object","null"]) producesjson: cannot unmarshal array into Go struct field .type of type string, and the backend then lists no tools at all. Nothing downstream can use or manage its other, perfectly valid tools. Observed in a downstream deployment connecting to a remote MCP server, where the surfaced error wasdecoding tools from tools/list: json: cannot unmarshal array into Go struct field .type of type string.modernListAllnow decodes list items one at a time. An item that fails to decode is skipped with a warning naming it (name, orurifor resources), and every other item on the page is kept.Type of change
Test plan
task test)task test-e2e)task lint-fix)Details of what was run:
TestModernEnumerate_SkipsUndecodableTool(a page with a good tool, a tool with an arrayinputSchema.type, and another good tool returns the two good tools) andTestModernEnumerate_MalformedItemsFieldFails(a non-arraytoolsfield still fails). Before the change, the first test failed with exactly the error above.pkg/vmcp/client/...with the same flagstask testuses (-raceand the-ldflags=-extldflags=-Wl,-wworkaround), scoped to that package rather than the whole suite. It passes.task lint(0 issues); did not runtask lint-fixor the fulltask test/e2e suites.Does this introduce a user-facing change?
A backend that advertises a tool the compat types cannot decode now lists its remaining tools instead of none. The undecodable tool is absent from the aggregated list and a
skipping undecodable item in tools/list responsewarning names it. Backends whose tools all decode are unaffected.Special notes for reviewers
toolhive-core'smcpcompat/mcpToolArgumentsSchema, whoseTypeis astring; a follow-up there to accept a type array would keep such tools instead of skipping them. Skipping was chosen here because coercing the type would change the schema forwarded to clients.modernListAll.🤖 Generated with Claude Code