fix: classify mcp query failures (#302)

This commit is contained in:
Andrey Avtomonov 2026-06-15 14:48:24 +02:00 committed by GitHub
parent 8a50601582
commit 7e29543398
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 102 additions and 8 deletions

View file

@ -1,3 +1,5 @@
import { KtxQueryError } from '../../errors.js';
const MUTATING_SQL =
/^\s*(insert|update|delete|merge|alter|drop|create|truncate|grant|revoke|copy|call|do|vacuum|analyze|refresh)\b/i;
const READ_SQL = /^\s*(select|with)\b/i;
@ -80,7 +82,7 @@ function assertSingleSqlStatement(sql: string): void {
if (sql[index] === ';') {
sawSemicolon = true;
} else if (sawSemicolon && !/\s/.test(sql[index])) {
throw new Error('Only one SQL statement can be executed.');
throw new KtxQueryError('Only one SQL statement can be executed.');
}
index += 1;
}
@ -89,7 +91,7 @@ function assertSingleSqlStatement(sql: string): void {
export function assertReadOnlySql(sql: string): string {
const trimmed = stripLeadingSqlComments(sql).trim();
if (!READ_SQL.test(trimmed) || MUTATING_SQL.test(trimmed)) {
throw new Error('Only read-only SELECT/WITH queries can be executed locally.');
throw new KtxQueryError('Only read-only SELECT/WITH queries can be executed locally.');
}
assertSingleSqlStatement(trimmed);
return trimmed;
@ -133,7 +135,7 @@ export function limitSqlForExecution(sql: string, maxRows: number | undefined):
return trimmed;
}
if (!Number.isInteger(maxRows) || maxRows <= 0) {
throw new Error('maxRows must be a positive integer.');
throw new KtxQueryError('maxRows must be a positive integer.');
}
return `select * from (${trimmed}) as ktx_query_result limit ${maxRows}`;
}

View file

@ -10,7 +10,7 @@ import {
shouldEmitMcpTelemetry,
} from '../../telemetry/index.js';
import { collectTelemetryRedactionSecrets } from '../../telemetry/redaction-secrets.js';
import { scrubErrorClass } from '../../telemetry/scrubber.js';
import { formatErrorDetail, scrubErrorClass } from '../../telemetry/scrubber.js';
import type {
KtxMcpClientInfo,
KtxMcpContextPorts,
@ -564,6 +564,28 @@ function clientTelemetryFields(
};
}
// Tools registered via registerParsedTool catch their own errors and return an
// isError result, so the telemetry layer never sees the thrown Error. Recover
// the failure message from the result's text content (the same string the agent
// reads) so the outcome event is self-diagnosing.
function mcpErrorResultDetail(result: unknown): string | undefined {
if (typeof result !== 'object' || result === null || !('content' in result)) {
return undefined;
}
const content = (result as { content?: unknown }).content;
if (!Array.isArray(content)) {
return undefined;
}
const text = content
.map((block) =>
typeof block === 'object' && block !== null && typeof (block as { text?: unknown }).text === 'string'
? (block as { text: string }).text
: '',
)
.join('\n');
return formatErrorDetail(text);
}
function instrumentMcpServer(
server: KtxMcpServerLike,
telemetry: { projectDir?: string; io?: KtxCliIo; getClientInfo?: () => KtxMcpClientInfo | undefined },
@ -577,6 +599,7 @@ function instrumentMcpServer(
if (telemetry.io && telemetry.projectDir && shouldEmitMcpTelemetry()) {
const isError =
typeof result === 'object' && result !== null && 'isError' in result && result.isError === true;
const errorDetail = isError ? mcpErrorResultDetail(result) : undefined;
await emitTelemetryEvent({
name: 'mcp_request_completed',
projectDir: telemetry.projectDir,
@ -586,6 +609,7 @@ function instrumentMcpServer(
outcome: isError ? 'error' : 'ok',
durationMs: Math.max(0, performance.now() - startedAt),
sampleRate: mcpTelemetrySampleRate(),
...(errorDetail ? { errorDetail } : {}),
...clientTelemetryFields(telemetry.getClientInfo),
},
});
@ -608,6 +632,7 @@ function instrumentMcpServer(
}
if (telemetry.io && telemetry.projectDir && shouldEmitMcpTelemetry()) {
const errorClass = scrubErrorClass(error);
const errorDetail = formatErrorDetail(error);
await emitTelemetryEvent({
name: 'mcp_request_completed',
projectDir: telemetry.projectDir,
@ -616,6 +641,7 @@ function instrumentMcpServer(
toolName: name,
outcome: 'error',
...(errorClass ? { errorClass } : {}),
...(errorDetail ? { errorDetail } : {}),
durationMs: Math.max(0, performance.now() - startedAt),
sampleRate: mcpTelemetrySampleRate(),
...clientTelemetryFields(telemetry.getClientInfo),

View file

@ -1,6 +1,6 @@
import type { KtxSqlQueryExecutorPort } from '../../context/connections/query-executor.js';
import { resolveConfiguredConnection } from '../../context/connections/resolve-connection.js';
import { KtxQueryError, isNativeProgrammingFault } from '../../errors.js';
import { KtxExpectedError, KtxQueryError, isNativeProgrammingFault } from '../../errors.js';
import { localConnectionInfoFromConfig } from '../../context/connections/local-warehouse-descriptor.js';
import type { KtxEmbeddingPort } from '../../context/core/embedding.js';
import type { KtxSemanticLayerComputePort } from '../../context/daemon/semantic-layer-compute.js';
@ -46,7 +46,9 @@ async function executeValidatedReadOnlySql(
}
const validation = await options.sqlAnalysis.validateReadOnly(input.sql, sqlAnalysisDialectForDriver(connection.driver));
if (!validation.ok) {
throw new Error(validation.error ?? 'SQL is not read-only.');
// A read-only guard rejecting the agent's SQL is an expected outcome, not a
// ktx fault: classify it so reportException keeps it out of Error Tracking.
throw new KtxQueryError(validation.error ?? 'SQL is not read-only.');
}
const createConnector = options.localScan?.createConnector;
if (!createConnector) {
@ -75,7 +77,7 @@ async function executeValidatedReadOnlySql(
// while preserving the warehouse's own diagnostics. A native JS error
// (TypeError, etc.) signals a bug in connector code — let it propagate
// unchanged so Error Tracking still sees it.
if (isNativeProgrammingFault(error)) {
if (isNativeProgrammingFault(error) || error instanceof KtxExpectedError) {
throw error;
}
throw new KtxQueryError(error instanceof Error ? error.message : String(error), { cause: error });

View file

@ -162,6 +162,7 @@
"outcome",
"durationMs",
"errorClass",
"errorDetail",
"sampleRate",
"mcpClientName",
"mcpClientVersion"
@ -1167,6 +1168,10 @@
"errorClass": {
"type": "string"
},
"errorDetail": {
"type": "string",
"maxLength": 1000
},
"sampleRate": {
"type": "number",
"const": 1

View file

@ -161,6 +161,7 @@ const mcpRequestCompletedSchema = telemetryCommonEnvelopeSchema
outcome: outcomeSchema,
durationMs: z.number().nonnegative(),
errorClass: z.string().optional(),
errorDetail: z.string().max(1000).optional(),
sampleRate: z.literal(1),
// Raw, client-tool-controlled identity from the MCP initialize handshake
// (clientInfo.name/version). Optional: clients may omit clientInfo. Stored
@ -349,7 +350,16 @@ export const telemetryEventCatalog = [
{
name: 'mcp_request_completed',
description: 'Emitted for sampled MCP tool requests.',
fields: ['toolName', 'outcome', 'durationMs', 'errorClass', 'sampleRate', 'mcpClientName', 'mcpClientVersion'],
fields: [
'toolName',
'outcome',
'durationMs',
'errorClass',
'errorDetail',
'sampleRate',
'mcpClientName',
'mcpClientVersion',
],
},
{
name: 'daemon_started',

View file

@ -1,4 +1,5 @@
import { describe, expect, it } from 'vitest';
import { KtxExpectedError, KtxQueryError } from '../../../src/errors.js';
import {
assertReadOnlySql,
limitSqlForExecution,
@ -20,6 +21,20 @@ describe('assertReadOnlySql', () => {
);
});
// A guard refusing the agent's SQL is an expected outcome; classifying it as
// KtxQueryError keeps reportException from filing it as a ktx fault.
it('rejects with an expected KtxQueryError, not a bare Error', () => {
expect(() => assertReadOnlySql('delete from orders')).toThrow(KtxQueryError);
expect(() => assertReadOnlySql('describe orders')).toThrow(KtxQueryError);
expect(() => assertReadOnlySql('select 1; drop table orders')).toThrow(KtxQueryError);
try {
assertReadOnlySql('describe orders');
expect.unreachable('expected a throw');
} catch (error) {
expect(error).toBeInstanceOf(KtxExpectedError);
}
});
it('accepts read-only queries that begin with leading comments', () => {
expect(assertReadOnlySql('-- daily widget sales\nselect count(*) from public.widget_sales')).toBe(
'select count(*) from public.widget_sales',

View file

@ -287,6 +287,35 @@ describe('createKtxMcpServer', () => {
expect(io.stderrText()).not.toContain('mcpClientVersion');
});
it('records the failure message as errorDetail when a tool returns an error', async () => {
vi.spyOn(Math, 'random').mockReturnValue(0);
vi.stubEnv('KTX_TELEMETRY_DEBUG', '1');
vi.stubEnv('CI', '');
const fake = makeFakeServer();
const io = makeIo();
createKtxMcpServer({
server: fake.server,
userContext: { userId: 'local-user' },
projectDir: '/tmp/ktx-mcp-error-detail',
io,
contextTools: {
knowledge: {
search: vi.fn<KtxKnowledgeMcpPort['search']>().mockRejectedValue(new Error('wiki search exploded')),
read: vi.fn<KtxKnowledgeMcpPort['read']>().mockResolvedValue(null),
},
},
});
await expect(getTool(fake.tools, 'wiki_search').handler({ query: 'revenue', limit: 5 })).resolves.toMatchObject({
isError: true,
});
expect(io.stderrText()).toContain('"event":"mcp_request_completed"');
expect(io.stderrText()).toContain('"outcome":"error"');
expect(io.stderrText()).toContain('"errorDetail":"wiki search exploded"');
});
it('reports MCP tool exceptions with a tool-derived source', async () => {
reportExceptionMock.mockClear();
vi.stubEnv('ANTHROPIC_API_KEY', 'mcp-anthropic-secret'); // pragma: allowlist secret

View file

@ -162,6 +162,7 @@
"outcome",
"durationMs",
"errorClass",
"errorDetail",
"sampleRate",
"mcpClientName",
"mcpClientVersion"
@ -1167,6 +1168,10 @@
"errorClass": {
"type": "string"
},
"errorDetail": {
"type": "string",
"maxLength": 1000
},
"sampleRate": {
"type": "number",
"const": 1