fix: classify MCP SQL query errors as expected (#285)

This commit is contained in:
Andrey Avtomonov 2026-06-10 11:42:31 +02:00 committed by GitHub
parent b076431b0a
commit 036a745fc1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 226 additions and 10 deletions

View file

@ -1,4 +1,5 @@
import type { KtxSqlQueryExecutorPort } from '../../context/connections/query-executor.js'; 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 { 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';
@ -110,14 +111,26 @@ async function executeValidatedReadOnlySql(
throw new Error(`Connection "${connectionId}" does not support read-only SQL execution.`); throw new Error(`Connection "${connectionId}" does not support read-only SQL execution.`);
} }
await onProgress?.({ progress: 0.3, message: 'Executing' }); await onProgress?.({ progress: 0.3, message: 'Executing' });
const result = await connector.executeReadOnly( const result = await connector
{ .executeReadOnly(
connectionId, {
sql: input.sql, connectionId,
maxRows: input.maxRows, sql: input.sql,
}, maxRows: input.maxRows,
{ runId: 'mcp-sql-execution' }, },
); { 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 = { const response = {
headers: result.headers, headers: result.headers,
...(result.headerTypes ? { headerTypes: result.headerTypes } : {}), ...(result.headerTypes ? { headerTypes: result.headerTypes } : {}),

View file

@ -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
);
}

View file

@ -1,6 +1,7 @@
import { inspect } from 'node:util'; import { inspect } from 'node:util';
import { getKtxCliPackageInfo, type KtxCliIo, type KtxCliPackageInfo } from '../cli-runtime.js'; import { getKtxCliPackageInfo, type KtxCliIo, type KtxCliPackageInfo } from '../cli-runtime.js';
import { KtxExpectedError } from '../errors.js';
import { buildCommonEnvelope } from './events.js'; import { buildCommonEnvelope } from './events.js';
import { trackTelemetryException } from './emitter.js'; import { trackTelemetryException } from './emitter.js';
import { computeTelemetryProjectId, loadTelemetryIdentity } from './identity.js'; import { computeTelemetryProjectId, loadTelemetryIdentity } from './identity.js';
@ -39,6 +40,17 @@ function consumeHandledPrimitive(value: unknown): boolean {
return true; 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 { function shouldSkipAsAlreadyReported(error: unknown, handled: boolean): boolean {
if ((typeof error === 'object' || typeof error === 'function') && error !== null) { if ((typeof error === 'object' || typeof error === 'function') && error !== null) {
if (reportedObjects.has(error)) { if (reportedObjects.has(error)) {
@ -151,6 +163,9 @@ export async function reportException(input: {
redactionSecrets?: ReadonlyArray<string>; redactionSecrets?: ReadonlyArray<string>;
}): Promise<void> { }): Promise<void> {
try { try {
if (isExpectedError(input.error)) {
return;
}
if (shouldSkipAsAlreadyReported(input.error, input.context.handled)) { if (shouldSkipAsAlreadyReported(input.error, input.context.handled)) {
return; return;
} }

View file

@ -3,6 +3,7 @@ import { tmpdir } from 'node:os';
import { join } from 'node:path'; import { join } from 'node:path';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { initKtxProject } from '../../../src/context/project/project.js'; 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 { createKtxConnectorCapabilities, type KtxQueryResult, type KtxScanConnector, type KtxSchemaSnapshot } from '../../../src/context/scan/types.js';
import { writeLocalSlSource } from '../../../src/context/sl/local-sl.js'; import { writeLocalSlSource } from '../../../src/context/sl/local-sl.js';
import { createLocalProjectMcpContextPorts } from '../../../src/context/mcp/local-project-ports.js'; import { createLocalProjectMcpContextPorts } from '../../../src/context/mcp/local-project-ports.js';
@ -325,6 +326,78 @@ describe('createLocalProjectMcpContextPorts', () => {
expect(connector.executeReadOnly).not.toHaveBeenCalled(); 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 () => { it('exposes local scan entity details through MCP ports', async () => {
const project = await initKtxProject({ projectDir: tempDir }); const project = await initKtxProject({ projectDir: tempDir });
project.config.connections.warehouse = { project.config.connections.warehouse = {

View file

@ -2,8 +2,10 @@ import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises';
import { tmpdir } from 'node:os'; import { tmpdir } from 'node:os';
import { join } from 'node:path'; import { join } from 'node:path';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { z } from 'zod';
import type { KtxCliIo } from '../../src/cli-runtime.js'; import type { KtxCliIo } from '../../src/cli-runtime.js';
import { KtxExpectedError, KtxQueryError } from '../../src/errors.js';
import { __resetTelemetryEmitterForTests } from '../../src/telemetry/emitter.js'; import { __resetTelemetryEmitterForTests } from '../../src/telemetry/emitter.js';
import { import {
__resetTelemetryExceptionStateForTests, __resetTelemetryExceptionStateForTests,
@ -150,6 +152,71 @@ describe('reportException', () => {
expect((captures[0] as { properties: Record<string, unknown> }).properties.$groups).toBeUndefined(); expect((captures[0] as { properties: Record<string, unknown> }).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 () => { it('uses captureExceptionImmediate for fatal reports', async () => {
const { io } = makeIo(); const { io } = makeIo();

4
uv.lock generated
View file

@ -466,7 +466,7 @@ wheels = [
[[package]] [[package]]
name = "ktx-daemon" name = "ktx-daemon"
version = "0.10.0" version = "0.11.0"
source = { editable = "python/ktx-daemon" } source = { editable = "python/ktx-daemon" }
dependencies = [ dependencies = [
{ name = "fastapi" }, { name = "fastapi" },
@ -523,7 +523,7 @@ dev = [
[[package]] [[package]]
name = "ktx-sl" name = "ktx-sl"
version = "0.10.0" version = "0.11.0"
source = { editable = "python/ktx-sl" } source = { editable = "python/ktx-sl" }
dependencies = [ dependencies = [
{ name = "pydantic" }, { name = "pydantic" },