diff --git a/packages/cli/src/context/connections/read-only-sql.ts b/packages/cli/src/context/connections/read-only-sql.ts index cd2ab0b1..1bde80b1 100644 --- a/packages/cli/src/context/connections/read-only-sql.ts +++ b/packages/cli/src/context/connections/read-only-sql.ts @@ -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}`; } diff --git a/packages/cli/src/context/mcp/context-tools.ts b/packages/cli/src/context/mcp/context-tools.ts index 71ed3a14..28e7a9c7 100644 --- a/packages/cli/src/context/mcp/context-tools.ts +++ b/packages/cli/src/context/mcp/context-tools.ts @@ -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), diff --git a/packages/cli/src/context/mcp/local-project-ports.ts b/packages/cli/src/context/mcp/local-project-ports.ts index d3106593..7c2863cb 100644 --- a/packages/cli/src/context/mcp/local-project-ports.ts +++ b/packages/cli/src/context/mcp/local-project-ports.ts @@ -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 }); diff --git a/packages/cli/src/telemetry/events.schema.json b/packages/cli/src/telemetry/events.schema.json index c6c3d6f8..405f0240 100644 --- a/packages/cli/src/telemetry/events.schema.json +++ b/packages/cli/src/telemetry/events.schema.json @@ -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 diff --git a/packages/cli/src/telemetry/events.ts b/packages/cli/src/telemetry/events.ts index cf650492..6d311ac5 100644 --- a/packages/cli/src/telemetry/events.ts +++ b/packages/cli/src/telemetry/events.ts @@ -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', diff --git a/packages/cli/test/context/connections/read-only-sql.test.ts b/packages/cli/test/context/connections/read-only-sql.test.ts index 4504a126..8a090c28 100644 --- a/packages/cli/test/context/connections/read-only-sql.test.ts +++ b/packages/cli/test/context/connections/read-only-sql.test.ts @@ -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', diff --git a/packages/cli/test/context/mcp/server.test.ts b/packages/cli/test/context/mcp/server.test.ts index 6b52bd66..96a4bd55 100644 --- a/packages/cli/test/context/mcp/server.test.ts +++ b/packages/cli/test/context/mcp/server.test.ts @@ -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().mockRejectedValue(new Error('wiki search exploded')), + read: vi.fn().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 diff --git a/python/ktx-daemon/src/ktx_daemon/telemetry/events.schema.json b/python/ktx-daemon/src/ktx_daemon/telemetry/events.schema.json index c6c3d6f8..405f0240 100644 --- a/python/ktx-daemon/src/ktx_daemon/telemetry/events.schema.json +++ b/python/ktx-daemon/src/ktx_daemon/telemetry/events.schema.json @@ -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