mirror of
https://github.com/Kaelio/ktx.git
synced 2026-06-22 08:38:08 +02:00
fix: classify mcp query failures (#302)
This commit is contained in:
parent
8a50601582
commit
7e29543398
8 changed files with 102 additions and 8 deletions
|
|
@ -1,3 +1,5 @@
|
||||||
|
import { KtxQueryError } from '../../errors.js';
|
||||||
|
|
||||||
const MUTATING_SQL =
|
const MUTATING_SQL =
|
||||||
/^\s*(insert|update|delete|merge|alter|drop|create|truncate|grant|revoke|copy|call|do|vacuum|analyze|refresh)\b/i;
|
/^\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;
|
const READ_SQL = /^\s*(select|with)\b/i;
|
||||||
|
|
@ -80,7 +82,7 @@ function assertSingleSqlStatement(sql: string): void {
|
||||||
if (sql[index] === ';') {
|
if (sql[index] === ';') {
|
||||||
sawSemicolon = true;
|
sawSemicolon = true;
|
||||||
} else if (sawSemicolon && !/\s/.test(sql[index])) {
|
} 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;
|
index += 1;
|
||||||
}
|
}
|
||||||
|
|
@ -89,7 +91,7 @@ function assertSingleSqlStatement(sql: string): void {
|
||||||
export function assertReadOnlySql(sql: string): string {
|
export function assertReadOnlySql(sql: string): string {
|
||||||
const trimmed = stripLeadingSqlComments(sql).trim();
|
const trimmed = stripLeadingSqlComments(sql).trim();
|
||||||
if (!READ_SQL.test(trimmed) || MUTATING_SQL.test(trimmed)) {
|
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);
|
assertSingleSqlStatement(trimmed);
|
||||||
return trimmed;
|
return trimmed;
|
||||||
|
|
@ -133,7 +135,7 @@ export function limitSqlForExecution(sql: string, maxRows: number | undefined):
|
||||||
return trimmed;
|
return trimmed;
|
||||||
}
|
}
|
||||||
if (!Number.isInteger(maxRows) || maxRows <= 0) {
|
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}`;
|
return `select * from (${trimmed}) as ktx_query_result limit ${maxRows}`;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -10,7 +10,7 @@ import {
|
||||||
shouldEmitMcpTelemetry,
|
shouldEmitMcpTelemetry,
|
||||||
} from '../../telemetry/index.js';
|
} from '../../telemetry/index.js';
|
||||||
import { collectTelemetryRedactionSecrets } from '../../telemetry/redaction-secrets.js';
|
import { collectTelemetryRedactionSecrets } from '../../telemetry/redaction-secrets.js';
|
||||||
import { scrubErrorClass } from '../../telemetry/scrubber.js';
|
import { formatErrorDetail, scrubErrorClass } from '../../telemetry/scrubber.js';
|
||||||
import type {
|
import type {
|
||||||
KtxMcpClientInfo,
|
KtxMcpClientInfo,
|
||||||
KtxMcpContextPorts,
|
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(
|
function instrumentMcpServer(
|
||||||
server: KtxMcpServerLike,
|
server: KtxMcpServerLike,
|
||||||
telemetry: { projectDir?: string; io?: KtxCliIo; getClientInfo?: () => KtxMcpClientInfo | undefined },
|
telemetry: { projectDir?: string; io?: KtxCliIo; getClientInfo?: () => KtxMcpClientInfo | undefined },
|
||||||
|
|
@ -577,6 +599,7 @@ function instrumentMcpServer(
|
||||||
if (telemetry.io && telemetry.projectDir && shouldEmitMcpTelemetry()) {
|
if (telemetry.io && telemetry.projectDir && shouldEmitMcpTelemetry()) {
|
||||||
const isError =
|
const isError =
|
||||||
typeof result === 'object' && result !== null && 'isError' in result && result.isError === true;
|
typeof result === 'object' && result !== null && 'isError' in result && result.isError === true;
|
||||||
|
const errorDetail = isError ? mcpErrorResultDetail(result) : undefined;
|
||||||
await emitTelemetryEvent({
|
await emitTelemetryEvent({
|
||||||
name: 'mcp_request_completed',
|
name: 'mcp_request_completed',
|
||||||
projectDir: telemetry.projectDir,
|
projectDir: telemetry.projectDir,
|
||||||
|
|
@ -586,6 +609,7 @@ function instrumentMcpServer(
|
||||||
outcome: isError ? 'error' : 'ok',
|
outcome: isError ? 'error' : 'ok',
|
||||||
durationMs: Math.max(0, performance.now() - startedAt),
|
durationMs: Math.max(0, performance.now() - startedAt),
|
||||||
sampleRate: mcpTelemetrySampleRate(),
|
sampleRate: mcpTelemetrySampleRate(),
|
||||||
|
...(errorDetail ? { errorDetail } : {}),
|
||||||
...clientTelemetryFields(telemetry.getClientInfo),
|
...clientTelemetryFields(telemetry.getClientInfo),
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
|
|
@ -608,6 +632,7 @@ function instrumentMcpServer(
|
||||||
}
|
}
|
||||||
if (telemetry.io && telemetry.projectDir && shouldEmitMcpTelemetry()) {
|
if (telemetry.io && telemetry.projectDir && shouldEmitMcpTelemetry()) {
|
||||||
const errorClass = scrubErrorClass(error);
|
const errorClass = scrubErrorClass(error);
|
||||||
|
const errorDetail = formatErrorDetail(error);
|
||||||
await emitTelemetryEvent({
|
await emitTelemetryEvent({
|
||||||
name: 'mcp_request_completed',
|
name: 'mcp_request_completed',
|
||||||
projectDir: telemetry.projectDir,
|
projectDir: telemetry.projectDir,
|
||||||
|
|
@ -616,6 +641,7 @@ function instrumentMcpServer(
|
||||||
toolName: name,
|
toolName: name,
|
||||||
outcome: 'error',
|
outcome: 'error',
|
||||||
...(errorClass ? { errorClass } : {}),
|
...(errorClass ? { errorClass } : {}),
|
||||||
|
...(errorDetail ? { errorDetail } : {}),
|
||||||
durationMs: Math.max(0, performance.now() - startedAt),
|
durationMs: Math.max(0, performance.now() - startedAt),
|
||||||
sampleRate: mcpTelemetrySampleRate(),
|
sampleRate: mcpTelemetrySampleRate(),
|
||||||
...clientTelemetryFields(telemetry.getClientInfo),
|
...clientTelemetryFields(telemetry.getClientInfo),
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,6 @@
|
||||||
import type { KtxSqlQueryExecutorPort } from '../../context/connections/query-executor.js';
|
import type { KtxSqlQueryExecutorPort } from '../../context/connections/query-executor.js';
|
||||||
import { resolveConfiguredConnection } from '../../context/connections/resolve-connection.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 { localConnectionInfoFromConfig } from '../../context/connections/local-warehouse-descriptor.js';
|
||||||
import type { KtxEmbeddingPort } from '../../context/core/embedding.js';
|
import type { KtxEmbeddingPort } from '../../context/core/embedding.js';
|
||||||
import type { KtxSemanticLayerComputePort } from '../../context/daemon/semantic-layer-compute.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));
|
const validation = await options.sqlAnalysis.validateReadOnly(input.sql, sqlAnalysisDialectForDriver(connection.driver));
|
||||||
if (!validation.ok) {
|
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;
|
const createConnector = options.localScan?.createConnector;
|
||||||
if (!createConnector) {
|
if (!createConnector) {
|
||||||
|
|
@ -75,7 +77,7 @@ async function executeValidatedReadOnlySql(
|
||||||
// while preserving the warehouse's own diagnostics. A native JS error
|
// while preserving the warehouse's own diagnostics. A native JS error
|
||||||
// (TypeError, etc.) signals a bug in connector code — let it propagate
|
// (TypeError, etc.) signals a bug in connector code — let it propagate
|
||||||
// unchanged so Error Tracking still sees it.
|
// unchanged so Error Tracking still sees it.
|
||||||
if (isNativeProgrammingFault(error)) {
|
if (isNativeProgrammingFault(error) || error instanceof KtxExpectedError) {
|
||||||
throw error;
|
throw error;
|
||||||
}
|
}
|
||||||
throw new KtxQueryError(error instanceof Error ? error.message : String(error), { cause: error });
|
throw new KtxQueryError(error instanceof Error ? error.message : String(error), { cause: error });
|
||||||
|
|
|
||||||
|
|
@ -162,6 +162,7 @@
|
||||||
"outcome",
|
"outcome",
|
||||||
"durationMs",
|
"durationMs",
|
||||||
"errorClass",
|
"errorClass",
|
||||||
|
"errorDetail",
|
||||||
"sampleRate",
|
"sampleRate",
|
||||||
"mcpClientName",
|
"mcpClientName",
|
||||||
"mcpClientVersion"
|
"mcpClientVersion"
|
||||||
|
|
@ -1167,6 +1168,10 @@
|
||||||
"errorClass": {
|
"errorClass": {
|
||||||
"type": "string"
|
"type": "string"
|
||||||
},
|
},
|
||||||
|
"errorDetail": {
|
||||||
|
"type": "string",
|
||||||
|
"maxLength": 1000
|
||||||
|
},
|
||||||
"sampleRate": {
|
"sampleRate": {
|
||||||
"type": "number",
|
"type": "number",
|
||||||
"const": 1
|
"const": 1
|
||||||
|
|
|
||||||
|
|
@ -161,6 +161,7 @@ const mcpRequestCompletedSchema = telemetryCommonEnvelopeSchema
|
||||||
outcome: outcomeSchema,
|
outcome: outcomeSchema,
|
||||||
durationMs: z.number().nonnegative(),
|
durationMs: z.number().nonnegative(),
|
||||||
errorClass: z.string().optional(),
|
errorClass: z.string().optional(),
|
||||||
|
errorDetail: z.string().max(1000).optional(),
|
||||||
sampleRate: z.literal(1),
|
sampleRate: z.literal(1),
|
||||||
// Raw, client-tool-controlled identity from the MCP initialize handshake
|
// Raw, client-tool-controlled identity from the MCP initialize handshake
|
||||||
// (clientInfo.name/version). Optional: clients may omit clientInfo. Stored
|
// (clientInfo.name/version). Optional: clients may omit clientInfo. Stored
|
||||||
|
|
@ -349,7 +350,16 @@ export const telemetryEventCatalog = [
|
||||||
{
|
{
|
||||||
name: 'mcp_request_completed',
|
name: 'mcp_request_completed',
|
||||||
description: 'Emitted for sampled MCP tool requests.',
|
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',
|
name: 'daemon_started',
|
||||||
|
|
|
||||||
|
|
@ -1,4 +1,5 @@
|
||||||
import { describe, expect, it } from 'vitest';
|
import { describe, expect, it } from 'vitest';
|
||||||
|
import { KtxExpectedError, KtxQueryError } from '../../../src/errors.js';
|
||||||
import {
|
import {
|
||||||
assertReadOnlySql,
|
assertReadOnlySql,
|
||||||
limitSqlForExecution,
|
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', () => {
|
it('accepts read-only queries that begin with leading comments', () => {
|
||||||
expect(assertReadOnlySql('-- daily widget sales\nselect count(*) from public.widget_sales')).toBe(
|
expect(assertReadOnlySql('-- daily widget sales\nselect count(*) from public.widget_sales')).toBe(
|
||||||
'select count(*) from public.widget_sales',
|
'select count(*) from public.widget_sales',
|
||||||
|
|
|
||||||
|
|
@ -287,6 +287,35 @@ describe('createKtxMcpServer', () => {
|
||||||
expect(io.stderrText()).not.toContain('mcpClientVersion');
|
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 () => {
|
it('reports MCP tool exceptions with a tool-derived source', async () => {
|
||||||
reportExceptionMock.mockClear();
|
reportExceptionMock.mockClear();
|
||||||
vi.stubEnv('ANTHROPIC_API_KEY', 'mcp-anthropic-secret'); // pragma: allowlist secret
|
vi.stubEnv('ANTHROPIC_API_KEY', 'mcp-anthropic-secret'); // pragma: allowlist secret
|
||||||
|
|
|
||||||
|
|
@ -162,6 +162,7 @@
|
||||||
"outcome",
|
"outcome",
|
||||||
"durationMs",
|
"durationMs",
|
||||||
"errorClass",
|
"errorClass",
|
||||||
|
"errorDetail",
|
||||||
"sampleRate",
|
"sampleRate",
|
||||||
"mcpClientName",
|
"mcpClientName",
|
||||||
"mcpClientVersion"
|
"mcpClientVersion"
|
||||||
|
|
@ -1167,6 +1168,10 @@
|
||||||
"errorClass": {
|
"errorClass": {
|
||||||
"type": "string"
|
"type": "string"
|
||||||
},
|
},
|
||||||
|
"errorDetail": {
|
||||||
|
"type": "string",
|
||||||
|
"maxLength": 1000
|
||||||
|
},
|
||||||
"sampleRate": {
|
"sampleRate": {
|
||||||
"type": "number",
|
"type": "number",
|
||||||
"const": 1
|
"const": 1
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue