From 036a745fc1ee0ddb48e8a344f49a0cb254a83056 Mon Sep 17 00:00:00 2001 From: Andrey Avtomonov Date: Wed, 10 Jun 2026 11:42:31 +0200 Subject: [PATCH] fix: classify MCP SQL query errors as expected (#285) --- .../src/context/mcp/local-project-ports.ts | 29 ++++++-- packages/cli/src/errors.ts | 48 ++++++++++++ packages/cli/src/telemetry/exception.ts | 15 ++++ .../context/mcp/local-project-ports.test.ts | 73 +++++++++++++++++++ packages/cli/test/telemetry/exception.test.ts | 67 +++++++++++++++++ uv.lock | 4 +- 6 files changed, 226 insertions(+), 10 deletions(-) create mode 100644 packages/cli/src/errors.ts diff --git a/packages/cli/src/context/mcp/local-project-ports.ts b/packages/cli/src/context/mcp/local-project-ports.ts index 4c820b14..8971c51d 100644 --- a/packages/cli/src/context/mcp/local-project-ports.ts +++ b/packages/cli/src/context/mcp/local-project-ports.ts @@ -1,4 +1,5 @@ import type { KtxSqlQueryExecutorPort } from '../../context/connections/query-executor.js'; +import { 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'; @@ -110,14 +111,26 @@ async function executeValidatedReadOnlySql( throw new Error(`Connection "${connectionId}" does not support read-only SQL execution.`); } await onProgress?.({ progress: 0.3, message: 'Executing' }); - const result = await connector.executeReadOnly( - { - connectionId, - sql: input.sql, - maxRows: input.maxRows, - }, - { runId: 'mcp-sql-execution' }, - ); + const result = await connector + .executeReadOnly( + { + connectionId, + sql: input.sql, + maxRows: input.maxRows, + }, + { runId: 'mcp-sql-execution' }, + ) + .catch((error: unknown) => { + // A warehouse/driver rejection (e.g. the agent's SQL failed to compile) + // is a surfaced operational outcome, not a ktx fault: mark it expected + // 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)) { + throw error; + } + throw new KtxQueryError(error instanceof Error ? error.message : String(error), { cause: error }); + }); const response = { headers: result.headers, ...(result.headerTypes ? { headerTypes: result.headerTypes } : {}), diff --git a/packages/cli/src/errors.ts b/packages/cli/src/errors.ts new file mode 100644 index 00000000..a418fd10 --- /dev/null +++ b/packages/cli/src/errors.ts @@ -0,0 +1,48 @@ +/** + * Marks an error as an expected operational outcome that ktx surfaces to its + * caller (a connected agent or the CLI user) rather than an unexpected ktx + * fault. Examples: invalid agent input, a warehouse rejecting a query, or a + * validation guard rejecting a request. + * + * `reportException` skips PostHog Error Tracking for these so the bug stream + * stays free of routine, caller-driven failures. The failure is still surfaced + * to the caller (as a tool-error result or CLI error) and still recorded by the + * outcome-tagged telemetry events, so no diagnostic signal is lost. + */ +export class KtxExpectedError extends Error { + constructor(message: string, options?: ErrorOptions) { + super(message, options); + this.name = 'KtxExpectedError'; + } +} + +/** + * A query was rejected at the warehouse/driver boundary — the warehouse refused + * to compile or run it, or a read-only guard rejected it. Reuses the underlying + * error's message so the caller still sees the original warehouse diagnostics, + * and keeps the driver error as `cause`. + */ +export class KtxQueryError extends KtxExpectedError { + constructor(message: string, options?: ErrorOptions) { + super(message, options); + this.name = 'KtxQueryError'; + } +} + +/** + * True for the native JavaScript error types that signal a programming fault — a + * bug in ktx code rather than an operational outcome. These are universal + * language invariants (a `TypeError` never means "the warehouse rejected the + * query"), so callers can use this to keep genuine faults out of the + * expected-error classification and let them reach Error Tracking unchanged. + */ +export function isNativeProgrammingFault(error: unknown): boolean { + return ( + error instanceof TypeError || + error instanceof RangeError || + error instanceof ReferenceError || + error instanceof SyntaxError || + error instanceof EvalError || + error instanceof URIError + ); +} diff --git a/packages/cli/src/telemetry/exception.ts b/packages/cli/src/telemetry/exception.ts index 0ce81244..0a852891 100644 --- a/packages/cli/src/telemetry/exception.ts +++ b/packages/cli/src/telemetry/exception.ts @@ -1,6 +1,7 @@ import { inspect } from 'node:util'; import { getKtxCliPackageInfo, type KtxCliIo, type KtxCliPackageInfo } from '../cli-runtime.js'; +import { KtxExpectedError } from '../errors.js'; import { buildCommonEnvelope } from './events.js'; import { trackTelemetryException } from './emitter.js'; import { computeTelemetryProjectId, loadTelemetryIdentity } from './identity.js'; @@ -39,6 +40,17 @@ function consumeHandledPrimitive(value: unknown): boolean { return true; } +/** + * Expected operational errors are surfaced to the caller and recorded by the + * outcome-tagged telemetry events; they are not ktx faults, so they never reach + * Error Tracking. ktx marks these with KtxExpectedError at the site that knows + * the rejection is expected — the classification is never inferred globally from + * a generic error type, which would also swallow internal/fatal faults. + */ +function isExpectedError(error: unknown): boolean { + return error instanceof KtxExpectedError; +} + function shouldSkipAsAlreadyReported(error: unknown, handled: boolean): boolean { if ((typeof error === 'object' || typeof error === 'function') && error !== null) { if (reportedObjects.has(error)) { @@ -151,6 +163,9 @@ export async function reportException(input: { redactionSecrets?: ReadonlyArray; }): Promise { try { + if (isExpectedError(input.error)) { + return; + } if (shouldSkipAsAlreadyReported(input.error, input.context.handled)) { return; } diff --git a/packages/cli/test/context/mcp/local-project-ports.test.ts b/packages/cli/test/context/mcp/local-project-ports.test.ts index afe85b4c..18cea01b 100644 --- a/packages/cli/test/context/mcp/local-project-ports.test.ts +++ b/packages/cli/test/context/mcp/local-project-ports.test.ts @@ -3,6 +3,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { initKtxProject } from '../../../src/context/project/project.js'; +import { KtxQueryError } from '../../../src/errors.js'; import { createKtxConnectorCapabilities, type KtxQueryResult, type KtxScanConnector, type KtxSchemaSnapshot } from '../../../src/context/scan/types.js'; import { writeLocalSlSource } from '../../../src/context/sl/local-sl.js'; import { createLocalProjectMcpContextPorts } from '../../../src/context/mcp/local-project-ports.js'; @@ -325,6 +326,78 @@ describe('createLocalProjectMcpContextPorts', () => { expect(connector.executeReadOnly).not.toHaveBeenCalled(); }); + it('wraps warehouse execution errors as KtxQueryError while preserving the diagnostic message', async () => { + const project = await initKtxProject({ projectDir: tempDir }); + project.config.connections.warehouse = { + driver: 'snowflake', + url: 'env:DATABASE_URL', + }; + const driverError = new Error("SQL compilation error:\nsyntax error line 4 at position 14 unexpected 'rows'."); + driverError.name = 'OperationFailedError'; + const connector: KtxScanConnector = { + ...testConnector(testSnapshot(), { headers: [], rows: [], totalRows: 0, rowCount: 0 }), + executeReadOnly: vi.fn(async () => { + throw driverError; + }), + }; + const sqlAnalysis = { + analyzeForFingerprint: vi.fn(), + analyzeBatch: vi.fn(), + validateReadOnly: vi.fn(async () => ({ ok: true, error: null })), + }; + const ports = createLocalProjectMcpContextPorts(project, { + sqlAnalysis, + localScan: { createConnector: vi.fn(async () => connector) }, + embeddingService: null, + }); + + const execution = ports.sqlExecution?.execute({ + connectionId: 'warehouse', + sql: 'select\n count(*)\nfrom events\nlimit 100 rows', + maxRows: 1000, + }); + + await expect(execution).rejects.toBeInstanceOf(KtxQueryError); + await expect(execution).rejects.toThrow("syntax error line 4 at position 14 unexpected 'rows'."); + await expect(execution).rejects.toMatchObject({ cause: driverError }); + expect(connector.cleanup).toHaveBeenCalled(); + }); + + it('lets connector programming faults propagate instead of masking them as query errors', async () => { + const project = await initKtxProject({ projectDir: tempDir }); + project.config.connections.warehouse = { + driver: 'snowflake', + url: 'env:DATABASE_URL', + }; + const bug = new TypeError("Cannot read properties of undefined (reading 'rows')"); + const connector: KtxScanConnector = { + ...testConnector(testSnapshot(), { headers: [], rows: [], totalRows: 0, rowCount: 0 }), + executeReadOnly: vi.fn(async () => { + throw bug; + }), + }; + const sqlAnalysis = { + analyzeForFingerprint: vi.fn(), + analyzeBatch: vi.fn(), + validateReadOnly: vi.fn(async () => ({ ok: true, error: null })), + }; + const ports = createLocalProjectMcpContextPorts(project, { + sqlAnalysis, + localScan: { createConnector: vi.fn(async () => connector) }, + embeddingService: null, + }); + + const execution = ports.sqlExecution?.execute({ + connectionId: 'warehouse', + sql: 'select 1', + maxRows: 10, + }); + + await expect(execution).rejects.toBe(bug); + await expect(execution).rejects.toBeInstanceOf(TypeError); + expect(connector.cleanup).toHaveBeenCalled(); + }); + it('exposes local scan entity details through MCP ports', async () => { const project = await initKtxProject({ projectDir: tempDir }); project.config.connections.warehouse = { diff --git a/packages/cli/test/telemetry/exception.test.ts b/packages/cli/test/telemetry/exception.test.ts index 01608935..a3b90115 100644 --- a/packages/cli/test/telemetry/exception.test.ts +++ b/packages/cli/test/telemetry/exception.test.ts @@ -2,8 +2,10 @@ import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { z } from 'zod'; import type { KtxCliIo } from '../../src/cli-runtime.js'; +import { KtxExpectedError, KtxQueryError } from '../../src/errors.js'; import { __resetTelemetryEmitterForTests } from '../../src/telemetry/emitter.js'; import { __resetTelemetryExceptionStateForTests, @@ -150,6 +152,71 @@ describe('reportException', () => { expect((captures[0] as { properties: Record }).properties.$groups).toBeUndefined(); }); + it('skips Error Tracking for expected operational errors', async () => { + const { io } = makeIo(); + + await reportException({ + error: new KtxExpectedError('expected operational rejection surfaced to the caller'), + context: { source: 'mcp:sql_execution', handled: true, fatal: false }, + io, + packageInfo: { name: '@kaelio/ktx', version: '0.0.0-test' }, + projectDir: join(homeDir, 'project'), + }); + + expect(captures).toEqual([]); + expect(immediateCaptures).toEqual([]); + }); + + it('skips Error Tracking for warehouse query errors surfaced to the agent', async () => { + const { io } = makeIo(); + const driverError = new Error("SQL compilation error:\nsyntax error line 4 at position 14 unexpected 'rows'."); + driverError.name = 'OperationFailedError'; + + await reportException({ + error: new KtxQueryError(driverError.message, { cause: driverError }), + context: { source: 'mcp:sql_execution', handled: true, fatal: false }, + io, + packageInfo: { name: '@kaelio/ktx', version: '0.0.0-test' }, + projectDir: join(homeDir, 'project'), + }); + + expect(captures).toEqual([]); + }); + + it('reports ZodError faults instead of skipping them globally', async () => { + const { io } = makeIo(); + let zodError: unknown; + try { + z.object({ maxRows: z.number() }).parse({ maxRows: 'not-a-number' }); + } catch (error) { + zodError = error; + } + + await reportException({ + error: zodError, + context: { source: 'uncaughtException', handled: false, fatal: true }, + io, + packageInfo: { name: '@kaelio/ktx', version: '0.0.0-test' }, + immediate: true, + }); + + expect(immediateCaptures).toHaveLength(1); + }); + + it('still reports unexpected faults', async () => { + const { io } = makeIo(); + + await reportException({ + error: new TypeError("Cannot read properties of undefined (reading 'rows')"), + context: { source: 'mcp:sql_execution', handled: true, fatal: false }, + io, + packageInfo: { name: '@kaelio/ktx', version: '0.0.0-test' }, + projectDir: join(homeDir, 'project'), + }); + + expect(captures).toHaveLength(1); + }); + it('uses captureExceptionImmediate for fatal reports', async () => { const { io } = makeIo(); diff --git a/uv.lock b/uv.lock index 1f35cc3e..924d9159 100644 --- a/uv.lock +++ b/uv.lock @@ -466,7 +466,7 @@ wheels = [ [[package]] name = "ktx-daemon" -version = "0.10.0" +version = "0.11.0" source = { editable = "python/ktx-daemon" } dependencies = [ { name = "fastapi" }, @@ -523,7 +523,7 @@ dev = [ [[package]] name = "ktx-sl" -version = "0.10.0" +version = "0.11.0" source = { editable = "python/ktx-sl" } dependencies = [ { name = "pydantic" },