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, ' +