fix: read semantic sources safely (#284)

* fix: read semantic sources safely

* test: retarget reindex per-scope error case to a broken manifest

Reading a broken standalone source was made non-fatal in de1f1a8d (it is
surfaced for repair instead of throwing), so the reindex per-scope error
test no longer captured an error. Point it at a corrupt manifest shard,
which is the remaining fatal read failure the per-scope catch must
isolate, and assert the captured error names the offending file.

* fix(sl): decouple semantic-layer file names from warehouse naming rules

The in-file `name:` field is now the sole source identity; the filename is
a derived label that never participates in identity. This removes the
"Unsafe semantic-layer source name" failure class entirely: any warehouse
identifier (Snowflake's uppercase SIGNED_UP, EVENT$LOG, dotted names) can
be read, overlaid, edited, and deleted.

- New `source-files.ts`: one total filename derivation (safe lowercase
  names verbatim; otherwise slug + sha256-hash suffix, immune to
  case-insensitive-filesystem collisions) and one by-name file resolver.
- Reads resolve by name everywhere; the path-from-name fast path and
  `assertSafeSourceName` are gone.
- Writes resolve-then-write: rewrites land on the file that declares the
  name (human renames survive); new sources get a derived filename; a
  derived path occupied by a different source fails instead of clobbering.
- `readSourceFile` returns null for missing files instead of forcing every
  caller to launder IO errors; `deleteSource` distinguishes manifest-backed
  sources from not-found instead of silently succeeding.
- `sl_write_source` accepts verbatim warehouse identifiers (snake_case is
  now a recommendation for new sources) and rejects sourceName/source.name
  mismatches; `sl_edit_source` rejects name-changing edits.
- Ingest projection commits, gate-repair allowlists, and touched-source
  derivation use resolved paths / in-file names instead of interpolating
  `<connId>/<name>.yaml`.
- Collapsed the five parallel path derivations and duplicated path-token
  helpers onto the shared module; dropped dead service methods.

* fix(sl): resolve sources by declared name end-to-end and gate warehouse SQL with the parser-backed validator

- Key broken/renamed semantic-layer files by their recoverable in-file
  name (slSourceNameForFile) so mid-edit sources stay reachable under
  their real identity in reads, listings, and search
- Derive finalization touched sources from composed-source diffs and
  recover deleted files' declared names from the pre-change commit
  instead of parsing hash-derived filenames
- Resolve revert/rollback paths against history (listFilesAtCommit) so
  human-renamed files are restored where they lived at preHead
- Validate ingest sql_execution through the daemon's sqlglot
  validateReadOnly in the connection's dialect, sharing one
  driver-to-dialect map (sql-analysis/dialect.ts) across MCP and ingest
- Harden the local read-only SQL backstop: accept leading comments,
  reject smuggled second statements, and strip trailing
  semicolons/comments before row-limit wrapping
This commit is contained in:
Andrey Avtomonov 2026-06-10 14:06:13 +02:00 committed by GitHub
parent 853f39a7c3
commit f3f893bf01
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
51 changed files with 1797 additions and 476 deletions

View file

@ -3,6 +3,7 @@ import { dirname, join, relative } from 'node:path';
import YAML from 'yaml';
import type { MemoryAction } from '../../../../context/memory/types.js';
import { rawSourcesDirForSync } from '../../raw-sources-paths.js';
import { isSlYamlPath } from '../../../sl/source-files.js';
import type { FinalizationOverrideReplay } from '../../types.js';
import { mergeUsagePreservingExternal } from '../live-database/manifest.js';
import { historicSqlEvidenceEnvelopeSchema, type HistoricSqlEvidenceEnvelope } from './evidence.js';
@ -251,7 +252,7 @@ export async function projectHistoricSqlEvidence(input: HistoricSqlProjectionInp
const patternEvidence = evidence.filter((entry): entry is HistoricSqlEvidenceEnvelope & { kind: 'pattern' } => entry.kind === 'pattern');
const schemaRoot = join(input.workdir, 'semantic-layer', input.connectionId, '_schema');
for (const file of (await walkFiles(schemaRoot)).filter((candidate) => candidate.endsWith('.yaml') || candidate.endsWith('.yml'))) {
for (const file of (await walkFiles(schemaRoot)).filter(isSlYamlPath)) {
const path = join(schemaRoot, file);
const before = await readFile(path, 'utf-8');
const shard = (YAML.parse(before) ?? {}) as ManifestShard;

View file

@ -2,7 +2,6 @@ import { mkdir, readFile, writeFile } from 'node:fs/promises';
import { dirname, join } from 'node:path';
import { z } from 'zod';
import type { AgentRunnerPort, KtxRuntimeToolSet } from '../../context/llm/runtime-port.js';
import type { TouchedSlSource } from '../../context/tools/touched-sl-sources.js';
import type { IngestTraceWriter } from './ingest-trace.js';
import { traceTimed } from './ingest-trace.js';
@ -149,11 +148,13 @@ function buildToolSet(input: {
export function finalGateRepairPaths(input: {
changedWikiPageKeys: string[];
touchedSlSources: TouchedSlSource[];
// Resolved by the caller: SL filenames are derived labels, so the repair
// allowlist must carry the real on-disk paths, not name-interpolated ones.
touchedSlSourcePaths: string[];
}): string[] {
return [
...new Set([
...input.touchedSlSources.map((source) => `semantic-layer/${source.connectionId}/${source.sourceName}.yaml`),
...input.touchedSlSourcePaths,
...input.changedWikiPageKeys.map((pageKey) => `wiki/global/${pageKey}.md`),
]),
].sort();

View file

@ -1,3 +1,4 @@
import { isSlYamlPath } from '../../context/sl/source-files.js';
import type { SemanticLayerSource } from '../../context/sl/types.js';
import type { TouchedSlSource } from '../../context/tools/touched-sl-sources.js';
import type { IngestReportFinalizationMismatch } from './reports.js';
@ -64,39 +65,36 @@ export function deriveFinalizationWikiPageKeys(paths: string[]): string[] {
);
}
export async function deriveFinalizationTouchedSources(
input: DeriveTouchedSourcesInput,
): Promise<DeriveTouchedSourcesResult> {
// Source identity is the in-file `name:`; filenames are derived labels (see
// source-files.ts), so a changed path — manifest shard or standalone file —
// cannot be mapped to a source by parsing its filename. Instead, every changed
// semantic-layer file is attributed through the before/after diff of its
// connection's composed sources. A changed file whose connection diff is empty
// cannot be attributed to any source and is surfaced as unresolved.
export function deriveFinalizationTouchedSources(input: DeriveTouchedSourcesInput): DeriveTouchedSourcesResult {
const touched = new Map<string, TouchedSlSource>();
const unresolvedPaths: string[] = [];
const pathsByConnection = new Map<string, string[]>();
for (const path of input.changedPaths) {
if (!path.startsWith('semantic-layer/') || !(path.endsWith('.yaml') || path.endsWith('.yml'))) {
if (!path.startsWith('semantic-layer/') || !isSlYamlPath(path)) {
continue;
}
const parts = path.split('/');
const connectionId = parts[1] ?? '';
const connectionId = path.split('/')[1] ?? '';
if (!connectionId) {
unresolvedPaths.push(path);
continue;
}
if (parts[2] !== '_schema') {
const fileName = parts.at(-1) ?? '';
const sourceName = fileName.replace(/\.ya?ml$/, '');
if (!sourceName) {
unresolvedPaths.push(path);
continue;
}
touched.set(`${connectionId}:${sourceName}`, { connectionId, sourceName });
continue;
}
pathsByConnection.set(connectionId, [...(pathsByConnection.get(connectionId) ?? []), path]);
}
for (const [connectionId, paths] of pathsByConnection) {
const changedNames = changedSourceNames(
input.beforeSourcesByConnection.get(connectionId) ?? [],
input.afterSourcesByConnection.get(connectionId) ?? [],
);
if (changedNames.length === 0) {
unresolvedPaths.push(path);
unresolvedPaths.push(...paths);
continue;
}
for (const sourceName of changedNames) {

View file

@ -8,6 +8,7 @@ import { createRuntimeToolDescriptorFromAiTool } from '../../context/llm/runtime
import type { KtxRuntimeToolSet } from '../../context/llm/runtime-port.js';
import type { CaptureSession, MemoryAction } from '../../context/memory/types.js';
import type { SemanticLayerService } from '../../context/sl/semantic-layer.service.js';
import { isSlYamlPath, slSourceFilePath, slSourceNameForFile, sourceNameFromPath } from '../../context/sl/source-files.js';
import type { SemanticLayerSource } from '../../context/sl/types.js';
import type { SlValidationDeps } from '../../context/sl/tools/sl-warehouse-validation.js';
import { createTouchedSlSources, type TouchedSlSource } from '../../context/tools/touched-sl-sources.js';
@ -498,7 +499,7 @@ export class IngestBundleRunner {
const files = await this.deps.semanticLayerService.listFilesForConnection(connectionId);
const names = files
.filter((f) => !f.startsWith('_schema/'))
.map((f) => f.replace(/\.yaml$/, ''))
.map((f) => sourceNameFromPath(f))
.sort((left, right) => left.localeCompare(right));
const body = names.length > 0 ? names.join('\n') : '(no sources yet)';
return `## ${connectionId}\n${body}`;
@ -791,14 +792,52 @@ export class IngestBundleRunner {
].sort();
}
private touchedSlSourcesFromPaths(paths: string[]): TouchedSlSource[] {
return paths
.filter((path) => path.startsWith('semantic-layer/') && path.endsWith('.yaml') && !path.includes('/_schema/'))
.map((path) => {
const [, connectionId, fileName] = path.split('/');
return { connectionId: connectionId ?? '', sourceName: (fileName ?? '').replace(/\.yaml$/, '') };
})
.filter((source) => source.connectionId.length > 0 && source.sourceName.length > 0);
private async touchedSlSourcesFromPaths(
worktree: IngestSessionWorktree,
paths: string[],
deletedFileSha: string,
): Promise<TouchedSlSource[]> {
const sources: TouchedSlSource[] = [];
for (const path of paths) {
if (!path.startsWith('semantic-layer/') || !isSlYamlPath(path) || path.includes('/_schema/')) {
continue;
}
const [, connectionId] = path.split('/');
if (!connectionId) {
continue;
}
// Source identity is the in-file `name:`, never the filename — an uppercase
// warehouse source like `WIDGET_SALES` lives in a hash-derived
// `widget_sales-<hash>.yaml`, so parsing the basename yields a phantom name.
// Read the live file; when it was deleted this run, recover its declared
// name from the pre-change commit the way `revertSourceToPreHead` resolves a
// gone file from history. The filename is a last resort only when the content
// is unrecoverable from both.
let content: string | null;
try {
content = await readFile(join(worktree.workdir, path), 'utf-8');
} catch {
content = await worktree.git.getFileAtCommit(path, deletedFileSha).catch(() => null);
}
const sourceName = content === null ? sourceNameFromPath(path) : slSourceNameForFile(path, content);
if (sourceName.length > 0) {
sources.push({ connectionId, sourceName });
}
}
return sources;
}
// Inverse direction for commits and repair allowlists: resolve each touched
// source to its real on-disk path, falling back to the writer's derived
// filename when the file was deleted in this run.
private async touchedSlSourcePaths(workdir: string, touched: TouchedSlSource[]): Promise<string[]> {
const service = this.deps.semanticLayerService.forWorktree(workdir);
const paths: string[] = [];
for (const source of touched) {
const file = await service.readSourceFile(source.connectionId, source.sourceName);
paths.push(file?.path ?? slSourceFilePath(source.connectionId, source.sourceName));
}
return paths;
}
private touchedSlSourcesFromActions(actions: MemoryAction[], fallbackConnectionId: string): TouchedSlSource[] {
@ -1558,7 +1597,7 @@ export class IngestBundleRunner {
projectionTouchedSources = projection.touchedSources;
projectionChangedWikiPageKeys = projection.changedWikiPageKeys;
const projectionPaths = [
...projection.touchedSources.map((source) => `semantic-layer/${source.connectionId}/${source.sourceName}.yaml`),
...(await this.touchedSlSourcePaths(sessionWorktree.workdir, projection.touchedSources)),
...projection.changedWikiPageKeys.map((pageKey) => `wiki/global/${pageKey}.md`),
];
projectionTouchedPaths = projectionPaths;
@ -1740,7 +1779,11 @@ export class IngestBundleRunner {
await validateFinalIngestArtifacts({
connectionIds: slConnectionIds,
changedWikiPageKeys: this.wikiPageKeysFromPaths(touchedPaths),
touchedSlSources: this.touchedSlSourcesFromPaths(touchedPaths),
touchedSlSources: await this.touchedSlSourcesFromPaths(
sessionWorktree,
touchedPaths,
await sessionWorktree.git.revParseHead(),
),
wikiService: this.deps.wikiService.forWorktree(sessionWorktree.workdir),
semanticLayerService: this.deps.semanticLayerService.forWorktree(sessionWorktree.workdir),
validateTouchedSources: (touched) =>
@ -2289,20 +2332,34 @@ export class IngestBundleRunner {
)
: [];
const changedConnectionIds = [
...new Set([
...slConnectionIds,
...finalizationTouchedPaths
.filter((path) => path.startsWith('semantic-layer/'))
.map((path) => path.split('/')[1])
.filter((connectionId): connectionId is string => Boolean(connectionId)),
]),
].sort();
// Validate the write scope before deriving touched sources: attribution
// by before/after diff is only defined for connections whose
// pre-finalization snapshot was loaded (slConnectionIds), and an
// out-of-scope write would otherwise surface downstream as a bogus
// unresolved-path or declaration-mismatch failure instead of the real
// policy violation.
await traceTimed(
runTrace,
'finalization',
'semantic_layer_target_policy',
{
sourceKey: job.sourceKey,
allowedTargetConnectionIds: slConnectionIds,
touchedPaths: [...new Set(finalizationTouchedPaths)].sort(),
},
async () => {
assertSemanticLayerTargetPathsAllowed({
paths: finalizationTouchedPaths,
allowedConnectionIds: new Set(slConnectionIds),
});
},
);
const postFinalizationSourcesByConnection = await this.loadSourcesByConnection(
sessionWorktree.workdir,
changedConnectionIds,
slConnectionIds,
);
const scope = await deriveFinalizationTouchedSources({
const scope = deriveFinalizationTouchedSources({
changedPaths: finalizationTouchedPaths,
beforeSourcesByConnection: preFinalizationSourcesByConnection,
afterSourcesByConnection: postFinalizationSourcesByConnection,
@ -2437,7 +2494,7 @@ export class IngestBundleRunner {
...(isolatedDiffEnabled ? projectionTouchedSources : []),
...workUnitOutcomes.flatMap((outcome) => outcome.touchedSlSources),
...this.touchedSlSourcesFromActions(reconcileActions, job.connectionId),
...this.touchedSlSourcesFromPaths(postReconciliationPaths),
...(await this.touchedSlSourcesFromPaths(sessionWorktree, postReconciliationPaths, preReconciliationSha)),
...finalizationTouchedSources,
]);
const finalWikiGateScope = await this.wikiPageKeysForFinalGates({
@ -2528,7 +2585,7 @@ export class IngestBundleRunner {
const gateError = this.errorMessage(error);
const repairPaths = finalGateRepairPaths({
changedWikiPageKeys: finalChangedWikiPageKeys,
touchedSlSources: finalTouchedSlSources,
touchedSlSourcePaths: await this.touchedSlSourcePaths(sessionWorktree.workdir, finalTouchedSlSources),
});
emitStageProgress('final_gates', 89, 'Repairing final artifact gates');
const gateRepair = await repairFinalGateFailure({

View file

@ -4,6 +4,7 @@ import { fileURLToPath } from 'node:url';
import YAML from 'yaml';
import { localConnectionInfoFromConfig } from '../../context/connections/local-warehouse-descriptor.js';
import type { KtxSqlQueryExecutorPort } from '../../context/connections/query-executor.js';
import type { SqlAnalysisPort } from '../../context/sql-analysis/ports.js';
import type { KtxEmbeddingPort } from '../../context/core/embedding.js';
import type { KtxLogger } from '../../context/core/config.js';
import { noopLogger } from '../../context/core/config.js';
@ -95,6 +96,7 @@ export interface CreateLocalBundleIngestRuntimeOptions {
memoryModel?: string;
semanticLayerCompute?: KtxSemanticLayerComputePort;
queryExecutor?: KtxSqlQueryExecutorPort;
sqlAnalysis?: SqlAnalysisPort;
jobIdFactory?: () => string;
logger?: KtxLogger;
embeddingProvider?: KtxEmbeddingProvider | null;
@ -271,16 +273,13 @@ class LocalShapeOnlySlValidator implements SlValidatorPort<SlValidationDeps> {
}
async validateSingleSource(deps: SlValidationDeps, connectionId: string, sourceName: string) {
let content: string;
try {
const file = await deps.semanticLayerService.readSourceFile(connectionId, sourceName);
content = file.content;
} catch (error) {
return this.validateComposedSource(deps, connectionId, sourceName, error);
const file = await deps.semanticLayerService.readSourceFile(connectionId, sourceName);
if (!file) {
return this.validateComposedSource(deps, connectionId, sourceName, 'no standalone or overlay file found');
}
try {
const parsed = YAML.parse(content) as unknown as Record<string, unknown>;
const parsed = YAML.parse(file.content) as unknown as Record<string, unknown>;
return this.validateParsedSource(sourceName, parsed);
} catch (error) {
return {
@ -519,6 +518,7 @@ class LocalIngestToolsetFactory implements IngestToolsetFactoryPort {
authorResolver: GitAuthorResolverPort;
slSourcesRepository: SlSourcesIndexPort;
connections: SlConnectionCatalogPort;
sqlAnalysis?: SqlAnalysisPort;
contextStore: SqliteContextEvidenceStore;
embedding: KtxEmbeddingPort;
}) {
@ -551,6 +551,7 @@ class LocalIngestToolsetFactory implements IngestToolsetFactoryPort {
const slDiscoverTool = new SlDiscoverTool(slDeps, { maxSources: 25, minRrfScore: 0, maxDetailedSources: 5 });
const warehouseVerificationTools = createWarehouseVerificationTools({
connections: deps.connections,
...(deps.sqlAnalysis ? { sqlAnalysis: deps.sqlAnalysis } : {}),
fallbackFileStore: deps.project.fileStore,
wikiSearchTool,
slDiscoverTool,
@ -699,6 +700,7 @@ export function createLocalBundleIngestRuntime(
authorResolver: new LocalAuthorResolver(),
slSourcesRepository,
connections,
...(options.sqlAnalysis ? { sqlAnalysis: options.sqlAnalysis } : {}),
contextStore,
embedding,
});

View file

@ -2,6 +2,7 @@ import { randomUUID } from 'node:crypto';
import { cp, mkdir, rm } from 'node:fs/promises';
import { isAbsolute, resolve } from 'node:path';
import type { KtxSqlQueryExecutorPort } from '../../context/connections/query-executor.js';
import type { SqlAnalysisPort } from '../../context/sql-analysis/ports.js';
import type { KtxLogger } from '../../context/core/config.js';
import { createAbortError, isAbortError } from '../../context/core/abort.js';
import type { KtxSemanticLayerComputePort } from '../../context/daemon/semantic-layer-compute.js';
@ -35,6 +36,7 @@ export interface RunLocalIngestOptions {
memoryModel?: string;
semanticLayerCompute?: KtxSemanticLayerComputePort;
queryExecutor?: KtxSqlQueryExecutorPort;
sqlAnalysis?: SqlAnalysisPort;
logger?: KtxLogger;
embeddingProvider?: import('../../llm/types.js').KtxEmbeddingProvider | null;
abortSignal?: AbortSignal;
@ -159,6 +161,7 @@ async function runScheduledPullJob(options: {
memoryModel?: string;
semanticLayerCompute?: KtxSemanticLayerComputePort;
queryExecutor?: KtxSqlQueryExecutorPort;
sqlAnalysis?: SqlAnalysisPort;
logger?: KtxLogger;
embeddingProvider?: import('../../llm/types.js').KtxEmbeddingProvider | null;
abortSignal?: AbortSignal;
@ -214,6 +217,7 @@ export async function runLocalIngest(options: RunLocalIngestOptions): Promise<Lo
memoryModel: options.memoryModel,
semanticLayerCompute: options.semanticLayerCompute,
queryExecutor: options.queryExecutor,
sqlAnalysis: options.sqlAnalysis,
logger: options.logger,
embeddingProvider: options.embeddingProvider,
abortSignal: options.abortSignal,
@ -397,6 +401,7 @@ export async function runLocalMetabaseIngest(
memoryModel: options.memoryModel,
semanticLayerCompute: options.semanticLayerCompute,
queryExecutor: options.queryExecutor,
sqlAnalysis: options.sqlAnalysis,
logger: options.logger,
embeddingProvider: options.embeddingProvider,
abortSignal: options.abortSignal,

View file

@ -1,5 +1,6 @@
import type { KtxFileStorePort } from '../../../core/file-store.js';
import type { SlConnectionCatalogPort } from '../../../sl/ports.js';
import type { SqlAnalysisPort } from '../../../sql-analysis/ports.js';
import { WarehouseCatalogService } from '../../../scan/warehouse-catalog.js';
import type { BaseTool, ToolContext } from '../../../tools/base-tool.js';
import { DiscoverDataTool } from './discover-data.tool.js';
@ -8,6 +9,7 @@ import { SqlExecutionTool } from './sql-execution.tool.js';
export function createWarehouseVerificationTools(deps: {
connections: SlConnectionCatalogPort;
sqlAnalysis?: SqlAnalysisPort;
fallbackFileStore: KtxFileStorePort;
wikiSearchTool: BaseTool;
slDiscoverTool: BaseTool;
@ -18,7 +20,7 @@ export function createWarehouseVerificationTools(deps: {
});
return [
new EntityDetailsTool(catalogFactory),
new SqlExecutionTool(deps.connections),
new SqlExecutionTool(deps.connections, deps.sqlAnalysis),
new DiscoverDataTool({
wikiSearchTool: deps.wikiSearchTool,
slDiscoverTool: deps.slDiscoverTool,

View file

@ -1,6 +1,8 @@
import { z } from 'zod';
import { assertReadOnlySql, limitSqlForExecution } from '../../../../context/connections/read-only-sql.js';
import type { SlConnectionCatalogPort } from '../../../../context/sl/ports.js';
import { sqlAnalysisDialectForDriver } from '../../../../context/sql-analysis/dialect.js';
import type { SqlAnalysisPort } from '../../../../context/sql-analysis/ports.js';
import { BaseTool, type ToolContext, type ToolOutput } from '../../../../context/tools/base-tool.js';
const sqlExecutionInputSchema = z.object({
@ -40,7 +42,10 @@ function markdownTable(headers: string[], rows: unknown[][], totalRows: number):
export class SqlExecutionTool extends BaseTool<typeof sqlExecutionInputSchema> {
readonly name = 'sql_execution';
constructor(private readonly connections: SlConnectionCatalogPort) {
constructor(
private readonly connections: SlConnectionCatalogPort,
private readonly sqlAnalysis?: SqlAnalysisPort,
) {
super();
}
@ -69,9 +74,24 @@ export class SqlExecutionTool extends BaseTool<typeof sqlExecutionInputSchema> {
};
}
if (!this.sqlAnalysis) {
throw new Error('sql_execution requires parser-backed SQL validation.');
}
let sql: string;
let wrappedSql: string;
try {
const connection = await this.connections.getConnectionById(input.connectionId);
if (!connection) {
throw new Error(`Connection not found: ${input.connectionId}`);
}
const validation = await this.sqlAnalysis.validateReadOnly(
input.sql,
sqlAnalysisDialectForDriver(connection.connectionType),
);
if (!validation.ok) {
throw new Error(validation.error ?? 'SQL is not read-only.');
}
sql = assertReadOnlySql(input.sql);
wrappedSql = limitSqlForExecution(sql, input.rowLimit);
} catch (error) {