feat(mcp): improve MCP tool quality — error hints, shared helpers, better messages

.changeset/mcp-tool-quality-improvements.md

@@ -0,0 +1,12 @@
+---
+'@hyperdx/api': patch
+---
+
+feat(mcp): improve MCP tool quality — error hints, shared helpers, better messages
+
+Extract duplicated ClickHouse error handling into a shared helper with
+pattern-matched error hints (DateTime64 casting, AS alias quoting, response
+size limits) so agents get actionable guidance on common failures. Add
+reusable mergeWhereIntoSelectItems() helper for consistent top-level where
+injection. Improve source/connection-not-found messages to suggest calling
+hyperdx_list_sources.

packages/api/src/mcp/__tests__/query.test.ts

@@ -1,4 +1,17 @@
-import { parseTimeRange } from '../tools/query/helpers';
+// Mock heavy dependencies that break in unit-test context (no ClickHouse/Mongo)
+jest.mock('@/models/source', () => ({}));
+jest.mock('@/controllers/sources', () => ({}));
+jest.mock('@/controllers/connection', () => ({}));
+jest.mock('@/utils/trimToolResponse', () => ({
+  trimToolResponse: (data: unknown) => ({ data, isTrimmed: false }),
+}));
+
+import {
+  errorHint,
+  mergeWhereIntoSelectItems,
+  parseTimeRange,
+} from '../tools/query/helpers';
+import { resolveOrderBy } from '../tools/query/table';
 
 describe('parseTimeRange', () => {
   it('should return default range (last 15 minutes) when no arguments provided', () => {
@@ -72,3 +85,249 @@ describe('parseTimeRange', () => {
     expect(result).toHaveProperty('error');
   });
 });
+
+// ─── mergeWhereIntoSelectItems ───────────────────────────────────────────────
+
+describe('mergeWhereIntoSelectItems', () => {
+  it('should return items unchanged when topWhere is empty', () => {
+    const items = [{ where: 'level:error', whereLanguage: 'lucene' as const }];
+    const { items: result, warnings } = mergeWhereIntoSelectItems(
+      items,
+      '',
+      'lucene',
+    );
+    expect(result).toBe(items); // same reference, not a copy
+    expect(warnings).toHaveLength(0);
+  });
+
+  it('should inject top-level where when item has no where', () => {
+    const items = [{ where: '', whereLanguage: 'lucene' as const }];
+    const { items: result, warnings } = mergeWhereIntoSelectItems(
+      items,
+      'service:api',
+      'lucene',
+    );
+    expect(result[0].where).toBe('service:api');
+    expect(result[0].whereLanguage).toBe('lucene');
+    expect(warnings).toHaveLength(0);
+  });
+
+  it('should AND-combine when languages match', () => {
+    const items = [{ where: 'level:error', whereLanguage: 'lucene' as const }];
+    const { items: result, warnings } = mergeWhereIntoSelectItems(
+      items,
+      'service:api',
+      'lucene',
+    );
+    expect(result[0].where).toBe('(service:api) AND (level:error)');
+    expect(result[0].whereLanguage).toBe('lucene');
+    expect(warnings).toHaveLength(0);
+  });
+
+  it('should AND-combine SQL filters when both use sql', () => {
+    const items = [
+      { where: 'StatusCode >= 500', whereLanguage: 'sql' as const },
+    ];
+    const { items: result, warnings } = mergeWhereIntoSelectItems(
+      items,
+      "ServiceName = 'api'",
+      'sql',
+    );
+    expect(result[0].where).toBe(
+      "(ServiceName = 'api') AND (StatusCode >= 500)",
+    );
+    expect(result[0].whereLanguage).toBe('sql');
+    expect(warnings).toHaveLength(0);
+  });
+
+  it('should skip item and emit warning when languages differ', () => {
+    const items = [
+      { where: 'StatusCode >= 500', whereLanguage: 'sql' as const },
+    ];
+    const { items: result, warnings } = mergeWhereIntoSelectItems(
+      items,
+      'service:api',
+      'lucene',
+    );
+    // Item's own where is kept unchanged
+    expect(result[0].where).toBe('StatusCode >= 500');
+    expect(result[0].whereLanguage).toBe('sql');
+    // A warning is emitted
+    expect(warnings).toHaveLength(1);
+    expect(warnings[0]).toContain('select[0]');
+    expect(warnings[0]).toContain('was NOT applied');
+  });
+
+  it('should handle mixed items — some merged, some skipped', () => {
+    const items = [
+      { where: 'level:error', whereLanguage: 'lucene' as const },
+      { where: 'StatusCode >= 500', whereLanguage: 'sql' as const },
+      { where: '', whereLanguage: 'lucene' as const },
+    ];
+    const { items: result, warnings } = mergeWhereIntoSelectItems(
+      items,
+      'service:api',
+      'lucene',
+    );
+    // Item 0: merged (both lucene)
+    expect(result[0].where).toBe('(service:api) AND (level:error)');
+    // Item 1: skipped (sql vs lucene)
+    expect(result[1].where).toBe('StatusCode >= 500');
+    // Item 2: injected (empty where)
+    expect(result[2].where).toBe('service:api');
+    expect(warnings).toHaveLength(1);
+    expect(warnings[0]).toContain('select[1]');
+  });
+});
+
+// ─── errorHint ───────────────────────────────────────────────────────────────
+
+describe('errorHint', () => {
+  it('should match DateTime64 conversion errors', () => {
+    const hint = errorHint(
+      "Cannot convert string '2025-01-01T00:00:00Z' to type DateTime64(9)",
+    );
+    expect(hint).not.toBeNull();
+    expect(hint).toContain('toDateTime64');
+  });
+
+  it('should match DateTime64 parse errors', () => {
+    const hint = errorHint(
+      "Cannot parse string '2025-01-01' as type DateTime64",
+    );
+    expect(hint).not.toBeNull();
+    expect(hint).toContain('toDateTime64');
+  });
+
+  it('should match AS alias syntax errors with word boundary', () => {
+    const hint = errorHint('Syntax error: unexpected token AS');
+    expect(hint).not.toBeNull();
+    expect(hint).toContain('alias');
+  });
+
+  it('should NOT match AS embedded in other words like DATABASE', () => {
+    const hint = errorHint("Syntax error: Unknown database 'FOOBAR'");
+    expect(hint).toBeNull();
+  });
+
+  it('should NOT match lowercase "as" in cast expressions', () => {
+    const hint = errorHint(
+      'Syntax error: cannot cast as Float64 near token...',
+    );
+    expect(hint).toBeNull();
+  });
+
+  it('should match V8 string length overflow', () => {
+    const hint = errorHint(
+      'response length exceeds the maximum allowed size of V8 String',
+    );
+    expect(hint).not.toBeNull();
+    expect(hint).toContain('LIMIT');
+  });
+
+  it('should return null for unrecognized errors', () => {
+    const hint = errorHint('Connection refused');
+    expect(hint).toBeNull();
+  });
+});
+
+// ─── resolveOrderBy ──────────────────────────────────────────────────────────
+
+describe('resolveOrderBy', () => {
+  it('should return undefined when orderBy is undefined', () => {
+    expect(resolveOrderBy(undefined, [{ aggFn: 'count' }])).toBeUndefined();
+  });
+
+  it('should pass through non-aggFn values unchanged', () => {
+    expect(resolveOrderBy('SpanName', [{ aggFn: 'count' }])).toBe('SpanName');
+  });
+
+  it('should keep orderBy when it matches an alias', () => {
+    expect(resolveOrderBy('Total', [{ aggFn: 'count', alias: 'Total' }])).toBe(
+      'Total',
+    );
+  });
+
+  it('should return canonical alias case for case-insensitive match', () => {
+    expect(resolveOrderBy('total', [{ aggFn: 'count', alias: 'Total' }])).toBe(
+      'Total',
+    );
+    expect(
+      resolveOrderBy('TOTAL DESC', [{ aggFn: 'count', alias: 'Total' }]),
+    ).toBe('Total DESC');
+  });
+
+  it('should synthesize count()', () => {
+    expect(resolveOrderBy('count', [{ aggFn: 'count' }])).toBe('count()');
+  });
+
+  it('should synthesize avg(Duration)', () => {
+    expect(
+      resolveOrderBy('avg', [{ aggFn: 'avg', valueExpression: 'Duration' }]),
+    ).toBe('avg(Duration)');
+  });
+
+  it('should synthesize quantile with level', () => {
+    expect(
+      resolveOrderBy('quantile', [
+        { aggFn: 'quantile', valueExpression: 'Duration', level: 0.99 },
+      ]),
+    ).toBe('quantile(0.99)(Duration)');
+  });
+
+  it('should prefer alias over synthesized expression', () => {
+    expect(
+      resolveOrderBy('count', [{ aggFn: 'count', alias: 'Total Rows' }]),
+    ).toBe('Total Rows');
+  });
+
+  it('should be case-insensitive for aggFn matching', () => {
+    expect(resolveOrderBy('Count', [{ aggFn: 'count' }])).toBe('count()');
+    expect(
+      resolveOrderBy('AVG', [{ aggFn: 'avg', valueExpression: 'Duration' }]),
+    ).toBe('avg(Duration)');
+  });
+
+  it('should synthesize count(DISTINCT ...) for count_distinct', () => {
+    expect(
+      resolveOrderBy('count_distinct', [
+        { aggFn: 'count_distinct', valueExpression: 'UserId' },
+      ]),
+    ).toBe('count(DISTINCT UserId)');
+  });
+
+  it('should synthesize count(DISTINCT ...) with direction', () => {
+    expect(
+      resolveOrderBy('count_distinct DESC', [
+        { aggFn: 'count_distinct', valueExpression: 'UserId' },
+      ]),
+    ).toBe('count(DISTINCT UserId) DESC');
+  });
+
+  it('should NOT synthesize for "none" aggFn', () => {
+    expect(
+      resolveOrderBy('none', [
+        { aggFn: 'none', valueExpression: 'Duration / 1e6' },
+      ]),
+    ).toBe('none');
+  });
+
+  it('should handle trailing ASC/DESC', () => {
+    expect(resolveOrderBy('count DESC', [{ aggFn: 'count' }])).toBe(
+      'count() DESC',
+    );
+    expect(
+      resolveOrderBy('avg ASC', [
+        { aggFn: 'avg', valueExpression: 'Duration' },
+      ]),
+    ).toBe('avg(Duration) ASC');
+  });
+
+  it('should pass through quantile without level unchanged', () => {
+    expect(
+      resolveOrderBy('quantile', [
+        { aggFn: 'quantile', valueExpression: 'Duration' },
+      ]),
+    ).toBe('quantile');
+  });
+});

packages/api/src/mcp/__tests__/queryTool.test.ts

@@ -138,6 +138,7 @@ describe('MCP Query Tools', () => {
       expect(props).toContain('sourceId');
       expect(props).toContain('where');
       expect(props).toContain('sampleSize');
+      expect(props).toContain('topN');
       expect(props).toContain('bodyExpression');
       expect(tool!.inputSchema.required).toContain('sourceId');
     });
@@ -370,14 +371,14 @@ describe('MCP Query Tools', () => {
       expect(result.isError).toBeFalsy();
       expect(result.content).toHaveLength(1);
       const output = JSON.parse(getFirstText(result));
-      expect(output).toHaveProperty('result');
-      expect(output.result).toHaveProperty('patterns');
-      expect(output.result).toHaveProperty('totalCount');
-      expect(output.result).toHaveProperty('sampledRows');
-      expect(output.result).toHaveProperty('sampleMultiplier');
-      expect(output.result).toHaveProperty('bodyColumn', 'Body');
-      expect(output.result).toHaveProperty('timeRange');
-      expect(Array.isArray(output.result.patterns)).toBe(true);
+      expect(output).toHaveProperty('summary');
+      expect(output).toHaveProperty('patterns');
+      expect(output.summary).toHaveProperty('totalCount');
+      expect(output.summary).toHaveProperty('sampledCount');
+      expect(output.summary).toHaveProperty('sampleMultiplier');
+      expect(output.summary).toHaveProperty('bodyColumn', 'Body');
+      expect(output.summary).toHaveProperty('timeRange');
+      expect(Array.isArray(output.patterns)).toBe(true);
     });
 
     it('should execute with explicit bodyExpression on trace source', async () => {
@@ -391,9 +392,9 @@ describe('MCP Query Tools', () => {
       expect(result.isError).toBeFalsy();
       expect(result.content).toHaveLength(1);
       const output = JSON.parse(getFirstText(result));
-      expect(output).toHaveProperty('result');
-      expect(Array.isArray(output.result.patterns)).toBe(true);
-      expect(output.result).toHaveProperty('bodyColumn', 'SpanName');
+      expect(output).toHaveProperty('summary');
+      expect(Array.isArray(output.patterns)).toBe(true);
+      expect(output.summary).toHaveProperty('bodyColumn', 'SpanName');
     });
 
     it('should accept custom bodyExpression on log source', async () => {
@@ -407,7 +408,7 @@ describe('MCP Query Tools', () => {
       expect(result.isError).toBeFalsy();
       expect(result.content).toHaveLength(1);
       const output = JSON.parse(getFirstText(result));
-      expect(output.result).toHaveProperty('bodyColumn', 'SeverityText');
+      expect(output.summary).toHaveProperty('bodyColumn', 'SeverityText');
     });
 
     it('should respect sampleSize parameter', async () => {
@@ -503,10 +504,11 @@ describe('MCP Query Tools', () => {
 
         expect(result.isError).toBeFalsy();
         const output = JSON.parse(getFirstText(result));
-        const { patterns, totalCount, sampledRows } = output.result;
+        const { patterns } = output;
+        const { totalCount, sampledCount } = output.summary;
 
         expect(totalCount).toBeGreaterThanOrEqual(31);
-        expect(sampledRows).toBeGreaterThanOrEqual(31);
+        expect(sampledCount).toBeGreaterThanOrEqual(31);
         expect(patterns.length).toBeGreaterThanOrEqual(1);
 
         // The most common pattern should contain <*> placeholders

packages/api/src/mcp/tools/query/eventDeltas.ts

@@ -526,13 +526,20 @@ export function registerEventDeltas(server: McpServer, context: McpContext) {
             'are dropped by default — set includeHidden:true to inspect them.',
         };
 
-        const trimmedOutput = trimToolResponse(output);
+        const { data: trimmedOutput, isTrimmed } = trimToolResponse(output);
+
+        const finalOutput = isTrimmed
+          ? {
+              ...trimmedOutput,
+              note: 'Result was trimmed for context size. Narrow the time range, add filters, or reduce topN/topValuesPerProperty to reduce data.',
+            }
+          : trimmedOutput;
 
         return {
           content: [
             {
               type: 'text' as const,
-              text: JSON.stringify(trimmedOutput, null, 2),
+              text: JSON.stringify(finalOutput, null, 2),
             },
           ],
         };