From aadcd3bff7180a3b5d8236d96bdee9d93406f609 Mon Sep 17 00:00:00 2001 From: Andrii Los Date: Fri, 21 Aug 2026 23:00:08 +0200 Subject: [PATCH] fix(mcp): advertise quantile level as a string enum `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()(...)` 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 #2925. The metaschema check passes non-string enums happily, which is why this survived that PR. Refs #2967 --- .changeset/fix-mcp-non-string-enum.md | 5 ++ packages/api/src/mcp/__tests__/query.test.ts | 38 ++++++++++ .../api/src/mcp/__tests__/toolSchemas.test.ts | 72 +++++++++++++++++++ .../api/src/mcp/tools/dashboards/schemas.ts | 8 ++- packages/api/src/mcp/tools/query/schemas.ts | 30 +++++++- 5 files changed, 148 insertions(+), 5 deletions(-) create mode 100644 .changeset/fix-mcp-non-string-enum.md diff --git a/.changeset/fix-mcp-non-string-enum.md b/.changeset/fix-mcp-non-string-enum.md new file mode 100644 index 0000000000..6c308cc752 --- /dev/null +++ b/.changeset/fix-mcp-non-string-enum.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/api': patch +--- + +Advertise the MCP quantile `level` field as a string enum so Gemini-backed clients can use the server at all. `z.union([z.literal(0.5), ...])` renders as `{ "type": "number", "enum": [0.5, 0.9, 0.95, 0.99] }`, and Gemini's function declarations only accept `enum` alongside `type: "string"` — so a client that forwards MCP tool schemas to the provider had its entire tool list rejected because of this one field, surfacing as a generic "trouble connecting to the model provider" error that named neither the tool nor the property. Affected `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. The external REST API's own `level` contract is untouched. A new test asserts that no advertised tool schema carries a non-string `enum` or an array-form `items`, complementing the draft-2020-12 metaschema check. diff --git a/packages/api/src/mcp/__tests__/query.test.ts b/packages/api/src/mcp/__tests__/query.test.ts index d3ee33684c..21a285c969 100644 --- a/packages/api/src/mcp/__tests__/query.test.ts +++ b/packages/api/src/mcp/__tests__/query.test.ts @@ -22,6 +22,7 @@ import { import { applyMetricSelectDefaults, getMetricSelectIssues, + mcpQuantileLevelSchema, validateMetricSelectItems, } from '@/mcp/tools/query/schemas'; import { resolveOrderBy } from '@/mcp/tools/query/table'; @@ -1258,3 +1259,40 @@ describe('assertSourceKindMatchesSelect', () => { expect(assertSourceKindMatchesSelect({ kind: 'trace' }, null)).toBeNull(); }); }); + +describe('mcpQuantileLevelSchema', () => { + // Advertised to MCP callers as a string enum because Gemini's function + // declarations reject `enum` on any non-string type, which takes down the + // whole tool list. Everything downstream still wants a number, so parsing + // has to coerce — and callers working from a cached schema still send the + // number. See #2967. + it.each([ + ['0.5', 0.5], + ['0.9', 0.9], + ['0.95', 0.95], + ['0.99', 0.99], + ])('parses the string %p to the number %p', (input, expected) => { + expect(mcpQuantileLevelSchema.parse(input)).toBe(expected); + }); + + it.each([0.5, 0.9, 0.95, 0.99])( + 'still accepts the numeric form %p', + input => { + expect(mcpQuantileLevelSchema.parse(input)).toBe(input); + }, + ); + + it.each([0.97, '0.97', 1, '1', 0, 'p95', '', null, true, {}])( + 'rejects %p', + input => { + expect(mcpQuantileLevelSchema.safeParse(input).success).toBe(false); + }, + ); + + it('is optional at the call site without swallowing bad values', () => { + const optional = mcpQuantileLevelSchema.optional(); + + expect(optional.parse(undefined)).toBeUndefined(); + expect(optional.safeParse('0.97').success).toBe(false); + }); +}); diff --git a/packages/api/src/mcp/__tests__/toolSchemas.test.ts b/packages/api/src/mcp/__tests__/toolSchemas.test.ts index 5b0f98a474..5ff25cb983 100644 --- a/packages/api/src/mcp/__tests__/toolSchemas.test.ts +++ b/packages/api/src/mcp/__tests__/toolSchemas.test.ts @@ -53,4 +53,76 @@ describe('MCP tool input schemas', () => { expect(invalid).toEqual([]); }); + + /** + * Draft-2020-12 conformance is necessary but not sufficient. Providers + * accept narrower subsets of JSON Schema for function declarations, and a + * schema can sail through the metaschema check above while still making a + * provider reject the whole tool list: + * + * - `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"`. + * - `items` as an array is the draft-07 tuple form. The check above + * already rejects it (2020-12 moved tuples to `prefixItems`), but it is + * asserted here too so the failure names the offending property rather + * than surfacing as an Ajv compile error. + * + * The failure mode is the same for both and it is nasty: one bad property + * blanks the entire tool list for that provider, and the user sees a + * generic "trouble connecting to the model provider" error that identifies + * neither the tool nor the field. See #2967. + */ + it('avoid JSON Schema constructs that providers reject in function declarations', async () => { + const client = await createTestClient(context); + const { tools } = await client.listTools(); + + expect(tools.length).toBeGreaterThan(0); + + const offenders = tools.flatMap(tool => walk(tool.inputSchema, tool.name)); + + expect(offenders).toEqual([]); + }); }); + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +function isUnknownArray(value: unknown): value is unknown[] { + return Array.isArray(value); +} + +/** JSON pointer path -> problem, for every offending node under `node`. */ +function walk(node: unknown, path: string): string[] { + if (isUnknownArray(node)) { + return node.flatMap((child, i) => walk(child, `${path}/${i}`)); + } + if (!isRecord(node)) return []; + + const found: string[] = []; + + const enumValues = node.enum; + if (isUnknownArray(enumValues)) { + const nonString = enumValues.filter(value => typeof value !== 'string'); + if (nonString.length > 0) { + found.push( + `${path}: enum on a non-string type (${JSON.stringify(nonString)}) — ` + + 'Gemini only accepts enum with type "string"', + ); + } + } + + if (isUnknownArray(node.items)) { + found.push( + `${path}: items is an array (draft-07 tuple form) — use a single ` + + 'schema with minItems/maxItems', + ); + } + + for (const [key, value] of Object.entries(node)) { + found.push(...walk(value, `${path}/${key}`)); + } + + return found; +} diff --git a/packages/api/src/mcp/tools/dashboards/schemas.ts b/packages/api/src/mcp/tools/dashboards/schemas.ts index eaff0ae129..b6d3921b59 100644 --- a/packages/api/src/mcp/tools/dashboards/schemas.ts +++ b/packages/api/src/mcp/tools/dashboards/schemas.ts @@ -16,10 +16,12 @@ import { } from '@hyperdx/common-utils/dist/types'; import { z } from 'zod'; -import { getMetricSelectIssues } from '@/mcp/tools/query/schemas'; +import { + getMetricSelectIssues, + mcpQuantileLevelSchema, +} from '@/mcp/tools/query/schemas'; import { QUERYABLE_METRIC_KINDS } from '@/mcp/tools/sources/metricKinds'; import { - externalQuantileLevelSchema, MAX_TAG_LENGTH, MAX_TAGS, objectIdSchema, @@ -209,7 +211,7 @@ const mcpTileSelectItemSchema = z 'Without an alias the UI shows the raw ClickHouse expression (e.g. count(), quantile(0.95)(Duration)) which is hard to read. ' + 'Heatmap select items are the only exception (no alias needed).', ), - level: externalQuantileLevelSchema + level: mcpQuantileLevelSchema .optional() .describe( 'Percentile level for aggFn="quantile". REQUIRED for histogram and exponential histogram metrics with aggFn:"quantile".', diff --git a/packages/api/src/mcp/tools/query/schemas.ts b/packages/api/src/mcp/tools/query/schemas.ts index f24707e524..f274f7e893 100644 --- a/packages/api/src/mcp/tools/query/schemas.ts +++ b/packages/api/src/mcp/tools/query/schemas.ts @@ -92,6 +92,33 @@ const mcpMetricTypeSchema = z "metrics, use clickstack_sql against the table in the source's metricTables.summary.", ); +/** + * Percentile levels for `aggFn: "quantile"`, advertised to MCP callers as a + * string enum rather than a union of numeric literals. + * + * `enum` on a non-string type is valid JSON Schema, and the equivalent + * `z.union([z.literal(0.5), ...])` renders as + * `{ "type": "number", "enum": [0.5, 0.9, 0.95, 0.99] }` — but Gemini's + * function-declaration subset only accepts `enum` alongside `type: "string"`. + * Clients that forward MCP tool schemas straight to the provider therefore + * get the ENTIRE tool list rejected because of this one field, and the user + * sees a generic connectivity error that names neither the tool nor the + * property. See https://github.com/hyperdxio/hyperdx/issues/2967. + * + * The wire type is the only thing that changes. Numeric input is still + * accepted for callers working from a cached schema, the value is coerced + * back to a number before it reaches any consumer, and out-of-set values are + * still rejected either way — so `level` remains a `number` everywhere + * downstream, including the `quantile()(...)` SQL synthesis and the + * stored dashboard config. + */ +export const mcpQuantileLevelSchema = z + .preprocess( + value => (typeof value === 'number' ? String(value) : value), + z.enum(['0.5', '0.9', '0.95', '0.99']), + ) + .transform(Number); + /** * Shared cross-field validation issues for MCP select items. Returns the * list of Zod issues a `.superRefine` callback should emit. Kept as a pure @@ -262,8 +289,7 @@ export const mcpSelectItemSchema = z.object({ 'Always set a short, human-readable alias (e.g. "Requests", "P95 Latency", "Error Rate"). ' + 'Without an alias the UI shows the raw ClickHouse expression (e.g. count(), quantile(0.95)(Duration)) which is hard to read.', ), - level: z - .union([z.literal(0.5), z.literal(0.9), z.literal(0.95), z.literal(0.99)]) + level: mcpQuantileLevelSchema .optional() .describe( 'Percentile level. Required when aggFn is "quantile" on a histogram or exponential histogram metric, ' +