fix(mcp): advertise quantile level as a string enum - #2971
Conversation
`enum` on a non-string type is valid JSON Schema, and every provider except
Gemini accepts it. Gemini's function declarations only allow `enum` alongside
`type: "string"`, so a client that forwards MCP tool schemas straight to the
provider gets its entire tool list rejected because of this one field. The
user sees "We're having trouble connecting to the model provider", which names
neither the tool nor the property.
`level` was the only offender, via
`z.union([z.literal(0.5), z.literal(0.9), z.literal(0.95), z.literal(0.99)])`,
which renders as `{ "type": "number", "enum": [0.5, 0.9, 0.95, 0.99] }`. It
reached four tools: clickstack_timeseries, clickstack_table,
clickstack_save_dashboard and clickstack_patch_dashboard.
Only the advertised wire type changes. Numeric input is still accepted for
callers working from a cached schema, the value is coerced back to a number
before any consumer sees it, and out-of-set values are still rejected either
way, so `level` stays a number downstream in both the `quantile(<level>)(...)`
SQL synthesis and the stored dashboard config. `externalQuantileLevelSchema`
is left alone because it is the external REST API's public contract.
Adds a test asserting no advertised tool schema carries a non-string `enum` or
an array-form `items`, alongside the draft-2020-12 check from hyperdxio#2925. The
metaschema check passes non-string enums happily, which is why this survived
that PR.
Refs hyperdxio#2967
🦋 Changeset detectedLatest commit: e861bfd The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@RIP21 is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @RIP21, thanks for the pull request! Before we review code from a first-time contributor we ask that a maintainer vouches for you, and you're not on our list yet. This PR stays open — it just isn't in the review queue until someone vouches. To get vouched, open an issue saying hello and what you're working on: https://github.com/hyperdxio/hyperdx/issues/new?template=introduce-yourself.md A maintainer will usually reply within a day or two, and then this PR gets picked up as normal. More detail in our contributing guide. |
Greptile SummaryThis PR changes MCP quantile levels to advertise string enum values for Gemini compatibility while coercing accepted string and legacy numeric inputs back to numbers.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/api/src/mcp/tools/query/schemas.ts | Adds the shared string-advertised quantile schema while preserving numeric parsed output and the existing allowlist. |
| packages/api/src/mcp/tools/dashboards/schemas.ts | Reuses the MCP-specific quantile schema for dashboard tile select items without changing the existing module dependency edge. |
| packages/api/src/mcp/tests/query.test.ts | Covers string and numeric coercion, invalid values, and optional-field behavior. |
| packages/api/src/mcp/tests/toolSchemas.test.ts | Adds recursive compatibility checks across advertised MCP tool schemas. |
| .changeset/fix-mcp-non-string-enum.md | Documents the Gemini compatibility fix and schedules an API patch release. |
Reviews (2): Last reviewed commit: "Merge branch 'main' into claude/mcp-non-..." | Re-trigger Greptile
Deep Review✅ No critical issues found. This is a tightly scoped, backward-compatible schema change. The new 🟡 P2 — recommended
🔵 P3 nitpicks (1)
Reviewers (4 of 9 returned before synthesis): maintainability, project-standards, learnings-researcher, previous-comments. Correctness, testing, api-contract, kieran-typescript, and agent-native were dispatched but had not returned at the synthesis cutoff; the orchestrator performed independent correctness and contract verification (coercion round-trip, downstream Testing gaps: The |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
/vouch @RIP21 |
Triggered by [comment](#2971 (comment)) from @brandon-pereira. Vouch: @RIP21
Fixes the Gemini half of #2967. The tuple-
itemshalf is already handled by #2925 and ships in the next release, so this PR is only the non-stringenum.Why
enumon a non-string type is valid JSON Schema, and every provider in the matrix on #2967 accepts it except Gemini, whose function declarations only allowenumalongsidetype: "string". A client that forwards MCP tool schemas straight to the provider therefore gets its entire tool list rejected because of one field, and the user sees:That names neither the tool nor the property, and retrying never helps. Detaching the MCP server is the only workaround today. Same shape of failure as #2925, different construct and a different provider.
levelis the only offender:It reaches four tools through
mcpSelectItemSchemaandmcpTileSelectItemSchema:clickstack_timeseries,clickstack_table,clickstack_save_dashboard,clickstack_patch_dashboard.What changed
mcpQuantileLevelSchemainmcp/tools/query/schemas.tsadvertises the string enum, and both MCP select-item schemas use it.Only the advertised wire type changes. The schema still accepts the numeric form, so a caller working from a cached schema keeps working; it coerces back to a number before any consumer sees it, so
levelstays anumberdownstream in both thequantile(<level>)(...)SQL synthesis intable.tsand the stored dashboard config; and out-of-set values are still rejected from either input form. If you would rather advertise strings only and reject numbers outright, say so and I will drop thepreprocess— I included it because you mentioned server-side coercion and it makes the change invisible to existing callers.externalQuantileLevelSchemainutils/zod.tsis deliberately untouched. It is the external REST API's public contract, and all the existing numericlevel:fixtures underrouters/external-api/still exercise it unchanged.Tests
Two additions, per suggestion #2 on the issue.
mcp/__tests__/toolSchemas.test.tsgets a sibling to the draft-2020-12 check: no advertised tool schema may carry a non-stringenumor an array-formitems. Reverting just the schema change makes it fail and name every offender by JSON pointer:The array-form
itemsassertion is redundant with the metaschema check on paper, since 2020-12 moved tuples toprefixItems. It is there because the metaschema failure surfaces as an Ajv compile error while this one points at the property, which is the difference between a five-minute fix and an afternoon. Worth noting the reverse too: the 2020-12 metaschema accepts a non-stringenumhappily, which is exactly why this survived #2925.mcp/__tests__/query.test.tsgets direct coverage ofmcpQuantileLevelSchema: both input forms parse to the number, out-of-set and wrong-typed values are rejected, and.optional()does not swallow a bad value.Verified on this branch:
yarn ci:unitinpackages/api— 45 suites, 747 tests, all passing before these additions; 749 with themyarn lint— 302 warnings, exactly at the--max-warnings 302budget, so no new lint debtnpx tsc --noEmit— cleanNote on process
I am not on the vouch list, so I expect the
needs-vouchlabel. I opened #2967 and @brandon-pereira invited this PR there, so hopefully that covers the introduction, but happy to open a hello issue if you would rather do it by the book.Branch is prefixed
claude/per the agent-generated convention in AGENTS.md. Written with Claude, reviewed and verified against a live Cursor session and the local suites by me. NoCo-Authored-Bytrailer, per the same guide.