mirror of
https://github.com/Kaelio/ktx.git
synced 2026-06-13 08:15:14 +02:00
fix: classify MCP SQL query errors as expected (#285)
This commit is contained in:
parent
b076431b0a
commit
036a745fc1
6 changed files with 226 additions and 10 deletions
|
|
@ -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 } : {}),
|
||||
|
|
|
|||
48
packages/cli/src/errors.ts
Normal file
48
packages/cli/src/errors.ts
Normal 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
|
||||
);
|
||||
}
|
||||
|
|
@ -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<string>;
|
||||
}): Promise<void> {
|
||||
try {
|
||||
if (isExpectedError(input.error)) {
|
||||
return;
|
||||
}
|
||||
if (shouldSkipAsAlreadyReported(input.error, input.context.handled)) {
|
||||
return;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 = {
|
||||
|
|
|
|||
|
|
@ -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<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 () => {
|
||||
const { io } = makeIo();
|
||||
|
||||
|
|
|
|||
4
uv.lock
generated
4
uv.lock
generated
|
|
@ -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" },
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue