Skip to content

Commit 8ecf029

Browse files
sahrizviclaude
sahrizvi
and
claude
committed
fix(validate): fold both halves for every SQL-plus-schema operation, schema files too
Codex review of #1343 (gpt-5.6-sol). - Only `validate` folded the SQL; the other operations that match SQL against a schema (lint, explain, check incl. lintDiff/base_sql, fix, policy, semantics, testgen, equivalence, rewrite, correct, grade, query_pii, column_lineage, track_lineage, complete, optimize_for_query, prune_schema) got the folded schema with unfolded SQL — the mismatch the fold exists to remove. `prepareSql` now hands every one of them the pair - The fold is explicit (`normalizeSchemaContext(ctx, { fold })`): schema-only operations (diff, export, fingerprint, classify, resolve_term) see the names as written - `schema_path` took the SQL half without the schema half: a JSON/YAML file is now normalised and folded like an inline context; a DDL file is loaded as-is and the SQL left alone - Lexer: BigQuery/Databricks backticks fold too; a doubled quote inside a name keeps it as written; dotted quoted names fold as one; `$tag$…$tag$` and `E'…'` (backslash escapes) are opaque spans - The tool decided `has_schema` on its own and reported "VALID" for a table-less context as if existence had been checked; the handler says it now. A failed schema load (`success: false`) is an engine failure, not a valid result - Tests for each; `isExistenceError` gets a code-only fixture Not changed: the fold is dialect-blind. On a lowercase-folding warehouse an uppercase metadata name can be a genuinely quoted identifier, and an unquoted reference to it then validates; the tool has no dialect input to scope by. Same class as the tradeoff already noted in `foldIdentifierCase`. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6
1 parent f288654 commit 8ecf029

4 files changed

Lines changed: 232 additions & 61 deletions

File tree

‎packages/opencode/src/altimate/native/altimate-core.ts‎

Lines changed: 52 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
import * as core from "@altimateai/altimate-core"
1212
import { EngineCoerce } from "./engine-coerce"
1313
import { register } from "./dispatcher"
14-
import { schemaOrEmpty, resolveSchema, SchemaResolver, schemaProvided, foldQuotedIdentifierCase } from "./schema-resolver"
14+
import { schemaOrEmpty, SchemaResolver, prepareSql, foldQuotedIdentifierCase } from "./schema-resolver"
1515
import type { AltimateCoreResult } from "./types"
1616

1717
// ---------------------------------------------------------------------------
@@ -98,14 +98,15 @@ export function registerAll(): void {
9898
// 1. altimate_core.validate
9999
register("altimate_core.validate", async (params) => {
100100
try {
101-
const hasSchema = schemaProvided(params.schema_path, params.schema_context)
102-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
103-
const raw = await core.validate(hasSchema ? foldQuotedIdentifierCase(params.sql) : params.sql, schema)
101+
const { sql, schema, hasSchema } = prepareSql(params.sql, params.schema_path, params.schema_context)
102+
const raw = await core.validate(sql, schema)
104103
const data = toData(raw)
105104
// altimate_change start — without a schema the engine still runs against the
106105
// `_empty_` placeholder and reports every table as missing. The tool promises that
107106
// existence checks are skipped when no schema is given, so those findings are
108107
// dropped here and `valid` is recomputed from what remains (syntax, dialect).
108+
// Said back to the tool, which used to decide it separately (and differently).
109+
;(data as Record<string, unknown>).has_schema = hasSchema
109110
const errors = (data as { errors?: unknown }).errors
110111
if (!hasSchema && Array.isArray(errors)) {
111112
const kept = errors.filter((err) => !isExistenceError(err))
@@ -122,8 +123,8 @@ export function registerAll(): void {
122123
// 2. altimate_core.lint
123124
register("altimate_core.lint", async (params) => {
124125
try {
125-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
126-
const raw = core.lint(params.sql, schema)
126+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
127+
const raw = core.lint(sql, schema)
127128
const data = toData(raw)
128129
return ok(true, data)
129130
} catch (e) {
@@ -177,8 +178,8 @@ export function registerAll(): void {
177178
// 5. altimate_core.explain
178179
register("altimate_core.explain", async (params) => {
179180
try {
180-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
181-
const raw = await core.explain(params.sql, schema)
181+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
182+
const raw = await core.explain(sql, schema)
182183
const data = toData(raw)
183184
return ok(true, data)
184185
} catch (e) {
@@ -189,32 +190,36 @@ export function registerAll(): void {
189190
// 6. altimate_core.check — composite: validate + lint + scan_sql
190191
register("altimate_core.check", async (params) => {
191192
try {
192-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
193+
const { sql, schema, hasSchema } = prepareSql(params.sql, params.schema_path, params.schema_context)
194+
// The base SQL is matched against the same schema by lintDiff, so it gets the same fold.
195+
const baseSql = params.base_sql && hasSchema ? foldQuotedIdentifierCase(params.base_sql) : params.base_sql
193196
// NOTE: validation is deliberately NOT diff-scoped against base_sql.
194197
// The engine validates fail-fast (only the FIRST error is reported), so
195198
// subtracting base errors can hide genuinely new breakage behind a
196199
// pre-existing one. Re-reporting a pre-existing error is the safe mode.
197-
const validation: Record<string, unknown> = toData(await core.validate(params.sql, schema))
200+
const validation: Record<string, unknown> = toData(await core.validate(sql, schema))
198201
// Diff-scoped lint: when a base SQL is supplied, core returns only the
199202
// findings the change INTRODUCED (pre-existing issues in the file are
200203
// dropped) — the structural comparison stays in the AST engine.
201204
const lintResult =
202-
params.base_sql && typeof core.lintDiff === "function"
205+
baseSql && typeof core.lintDiff === "function"
203206
? core.lintDiff(
204-
params.sql,
205-
params.base_sql,
207+
sql,
208+
baseSql,
206209
// lintDiff takes SchemaDefinition JSON — normalize flat agent
207210
// schemas too, or the whole composite throws "missing field tables".
208-
params.schema_context ? SchemaResolver.normalizeSchemaContext(params.schema_context) : undefined,
211+
params.schema_context
212+
? SchemaResolver.normalizeSchemaContext(params.schema_context, { fold: true })
213+
: undefined,
209214
)
210-
: core.lint(params.sql, schema)
215+
: core.lint(sql, schema)
211216
// Diff-scope safety like lint: threats present in the base SQL are
212217
// pre-existing, not introduced by this change. Subtract as a MULTISET on
213218
// (rule, matched_pattern) — one base occurrence consumes one head
214219
// occurrence, so a PR that ADDS a second identical injection still
215220
// reports it. Recompute safe/risk_score from the surviving threats so a
216221
// fully pre-existing threat set doesn't leave a stale unsafe verdict.
217-
let safety: Record<string, unknown> = toData(core.scanSql(params.sql))
222+
let safety: Record<string, unknown> = toData(core.scanSql(sql))
218223
if (params.base_sql) {
219224
try {
220225
const baseCounts = new Map<string, number>()
@@ -259,7 +264,7 @@ export function registerAll(): void {
259264
// must not fail the whole composite.
260265
let pii: Record<string, unknown>
261266
try {
262-
pii = toData(core.checkQueryPii(params.sql, schema))
267+
pii = toData(core.checkQueryPii(sql, schema))
263268
if (params.base_sql && Array.isArray(pii.pii_columns) && (pii.pii_columns as any[]).length) {
264269
try {
265270
// Pre-existing exposures are not introduced by this change. The
@@ -379,8 +384,8 @@ export function registerAll(): void {
379384
// 7. altimate_core.fix
380385
register("altimate_core.fix", async (params) => {
381386
try {
382-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
383-
const raw = await core.fix(params.sql, schema, params.max_iterations ?? undefined)
387+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
388+
const raw = await core.fix(sql, schema, params.max_iterations ?? undefined)
384389
const data = toData(raw)
385390
return ok(true, data)
386391
} catch (e) {
@@ -391,8 +396,8 @@ export function registerAll(): void {
391396
// 8. altimate_core.policy
392397
register("altimate_core.policy", async (params) => {
393398
try {
394-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
395-
const raw = await core.checkPolicy(params.sql, schema, params.policy_json)
399+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
400+
const raw = await core.checkPolicy(sql, schema, params.policy_json)
396401
const data = toData(raw)
397402
return ok(true, data)
398403
} catch (e) {
@@ -403,8 +408,8 @@ export function registerAll(): void {
403408
// 9. altimate_core.semantics
404409
register("altimate_core.semantics", async (params) => {
405410
try {
406-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
407-
const raw = await core.checkSemantics(params.sql, schema)
411+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
412+
const raw = await core.checkSemantics(sql, schema)
408413
const data = toData(raw)
409414
return ok(true, data)
410415
} catch (e) {
@@ -415,8 +420,8 @@ export function registerAll(): void {
415420
// 10. altimate_core.testgen
416421
register("altimate_core.testgen", async (params) => {
417422
try {
418-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
419-
const raw = core.generateTests(params.sql, schema)
423+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
424+
const raw = core.generateTests(sql, schema)
420425
return ok(true, toData(raw))
421426
} catch (e) {
422427
return fail(e)
@@ -426,14 +431,15 @@ export function registerAll(): void {
426431
// 11. altimate_core.equivalence
427432
register("altimate_core.equivalence", async (params) => {
428433
try {
429-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
434+
const one = prepareSql(params.sql1, params.schema_path, params.schema_context)
435+
const two = prepareSql(params.sql2, params.schema_path, params.schema_context)
430436
// Pass the optional dialect hint so dialect-specific compiled warehouse SQL
431437
// (e.g. Snowflake semi-structured `col:field`) parses and the pair is
432438
// decidable instead of abstaining on a syntax error. Supported since
433439
// [email protected]. dialectHint coerces "" (the ReviewConfig default)
434440
// to undefined: the engine throws on an unknown dialect "", and "" must
435441
// mean auto-detect, not a real dialect.
436-
const raw = await core.checkEquivalence(params.sql1, params.sql2, schema, EngineCoerce.dialectHint(params.dialect))
442+
const raw = await core.checkEquivalence(one.sql, two.sql, one.schema, EngineCoerce.dialectHint(params.dialect))
437443
const data = toData(raw)
438444
return ok(true, data)
439445
} catch (e) {
@@ -470,8 +476,8 @@ export function registerAll(): void {
470476
// 14. altimate_core.rewrite
471477
register("altimate_core.rewrite", async (params) => {
472478
try {
473-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
474-
const raw = core.rewrite(params.sql, schema)
479+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
480+
const raw = core.rewrite(sql, schema)
475481
return ok(true, toData(raw))
476482
} catch (e) {
477483
return fail(e)
@@ -481,8 +487,8 @@ export function registerAll(): void {
481487
// 15. altimate_core.correct
482488
register("altimate_core.correct", async (params) => {
483489
try {
484-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
485-
const raw = await core.correct(params.sql, schema)
490+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
491+
const raw = await core.correct(sql, schema)
486492
const data = toData(raw)
487493
return ok(true, data)
488494
} catch (e) {
@@ -493,8 +499,8 @@ export function registerAll(): void {
493499
// 16. altimate_core.grade
494500
register("altimate_core.grade", async (params) => {
495501
try {
496-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
497-
const raw = await core.evaluate(params.sql, schema)
502+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
503+
const raw = await core.evaluate(sql, schema)
498504
const data = toData(raw)
499505
// EvalResult embeds a full safety scan — redact its threat echoes too
500506
// (the CLI grade check renders nested threat messages).
@@ -521,8 +527,8 @@ export function registerAll(): void {
521527
// 18. altimate_core.query_pii
522528
register("altimate_core.query_pii", async (params) => {
523529
try {
524-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
525-
const raw = core.checkQueryPii(params.sql, schema)
530+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
531+
const raw = core.checkQueryPii(sql, schema)
526532
return ok(true, toData(raw))
527533
} catch (e) {
528534
return fail(e)
@@ -545,8 +551,8 @@ export function registerAll(): void {
545551
// 20. altimate_core.column_lineage
546552
register("altimate_core.column_lineage", async (params) => {
547553
try {
548-
const schema = resolveSchema(params.schema_path, params.schema_context)
549-
const raw = core.columnLineage(params.sql, EngineCoerce.dialectHint(params.dialect), schema ?? undefined)
554+
const { sql, schema, hasSchema } = prepareSql(params.sql, params.schema_path, params.schema_context)
555+
const raw = core.columnLineage(sql, EngineCoerce.dialectHint(params.dialect), hasSchema ? schema : undefined)
550556
return ok(true, toData(raw))
551557
} catch (e) {
552558
return fail(e)
@@ -556,8 +562,9 @@ export function registerAll(): void {
556562
// 21. altimate_core.track_lineage
557563
register("altimate_core.track_lineage", async (params) => {
558564
try {
559-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
560-
const raw = core.trackLineage(params.queries, schema)
565+
const { schema, hasSchema } = prepareSql("", params.schema_path, params.schema_context)
566+
const queries = hasSchema ? params.queries.map(foldQuotedIdentifierCase) : params.queries
567+
const raw = core.trackLineage(queries, schema)
561568
return ok(true, toData(raw))
562569
} catch (e) {
563570
return fail(e)
@@ -598,8 +605,8 @@ export function registerAll(): void {
598605
// 25. altimate_core.complete
599606
register("altimate_core.complete", async (params) => {
600607
try {
601-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
602-
const raw = core.complete(params.sql, params.cursor_pos, schema)
608+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
609+
const raw = core.complete(sql, params.cursor_pos, schema)
603610
return ok(true, toData(raw))
604611
} catch (e) {
605612
return fail(e)
@@ -620,8 +627,8 @@ export function registerAll(): void {
620627
// 27. altimate_core.optimize_for_query
621628
register("altimate_core.optimize_for_query", async (params) => {
622629
try {
623-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
624-
const raw = core.optimizeForQuery(params.sql, schema)
630+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
631+
const raw = core.optimizeForQuery(sql, schema)
625632
return ok(true, toData(raw))
626633
} catch (e) {
627634
return fail(e)
@@ -631,8 +638,8 @@ export function registerAll(): void {
631638
// 28. altimate_core.prune_schema
632639
register("altimate_core.prune_schema", async (params) => {
633640
try {
634-
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
635-
const raw = core.pruneSchema(params.sql, schema)
641+
const { sql, schema } = prepareSql(params.sql, params.schema_path, params.schema_context)
642+
const raw = core.pruneSchema(sql, schema)
636643
return ok(true, toData(raw))
637644
} catch (e) {
638645
return fail(e)

‎packages/opencode/src/altimate/native/schema-resolver.ts‎

Lines changed: 73 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@
1717
*/
1818

1919
import { Schema } from "@altimateai/altimate-core"
20+
import fs from "node:fs"
21+
import path from "node:path"
22+
import YAML from "yaml"
2023

2124
/**
2225
* Detect whether a schema_context object is in flat format or SchemaDefinition format.
@@ -140,23 +143,38 @@ export function foldSchemaCase(def: { tables: Record<string, any> }): { tables:
140143
* over, not searched. Mixed-case quoted names are the exact-match case and are untouched.
141144
*/
142145
export function foldQuotedIdentifierCase(sql: string): string {
143-
return sql.replace(/'(?:[^']|'')*'|--[^\n]*|\/\*[\s\S]*?\*\/|"([^"]*)"/g, (match, quoted?: string) => {
144-
if (quoted === undefined) return match
145-
return /^[A-Z_][A-Z0-9_$]*$/.test(quoted) ? `"${quoted.toLowerCase()}"` : match
146-
})
146+
// Skipped spans, in order: dollar-quoted strings ($$…$$, $tag$…$tag$), E'…' strings
147+
// with backslash escapes, ordinary '…' strings ('' doubles), line and block comments.
148+
// Then the two quoted-identifier forms: "…" (SQL) and `…` (BigQuery/Databricks), a
149+
// doubled quote inside either kept as written — that name is not a plain identifier.
150+
return sql.replace(
151+
/\$([A-Za-z_][A-Za-z0-9_]*)?\$[\s\S]*?\$\1\$|[eE]'(?:[^'\\]|\\[\s\S]|'')*'|'(?:[^']|'')*'|--[^\n]*|\/\*[\s\S]*?\*\/|"((?:[^"]|"")*)"|`((?:[^`]|``)*)`/g,
152+
(match, _tag, dq?: string, bq?: string) => {
153+
const quoted = dq ?? bq
154+
if (quoted === undefined) return match
155+
// A doubled quote inside the name (`"A""B"`) is not a plain identifier: kept.
156+
if (!/^[A-Z_][A-Z0-9_$.]*$/.test(quoted)) return match
157+
const mark = match[0]
158+
return `${mark}${quoted.toLowerCase()}${mark}`
159+
},
160+
)
147161
}
148162

149163
/**
150164
* Normalize a schema_context into SchemaDefinition JSON format.
151165
* Accepts both flat and SchemaDefinition formats.
152166
*/
153-
export function normalizeSchemaContext(ctx: Record<string, any>): string {
154-
return JSON.stringify(normalizedSchemaDefinition(ctx))
167+
export function normalizeSchemaContext(ctx: Record<string, any>, opts: { fold?: boolean } = {}): string {
168+
return JSON.stringify(normalizedSchemaDefinition(ctx, opts))
155169
}
156170

157-
function normalizedSchemaDefinition(ctx: Record<string, any>): { tables: Record<string, any> } {
171+
/** `fold` applies the identifier-case fold (see `foldIdentifierCase`). It is for
172+
* operations that match SQL against the schema — the engine's comparison rules are
173+
* what the fold answers to. Schema-only operations (diff, export, fingerprint) get
174+
* the names as the caller wrote them. */
175+
function normalizedSchemaDefinition(ctx: Record<string, any>, opts: { fold?: boolean } = {}): { tables: Record<string, any> } {
158176
const def = (isSchemaDefinitionFormat(ctx) ? ctx : flatToSchemaDefinition(ctx)) as { tables: Record<string, any> }
159-
return foldSchemaCase(def)
177+
return opts.fold ? foldSchemaCase(def) : def
160178
}
161179

162180
/**
@@ -191,6 +209,52 @@ export function resolveSchema(
191209
return null
192210
}
193211

212+
/** What an operation that matches SQL against a schema runs on. */
213+
export interface PreparedSql {
214+
sql: string
215+
schema: Schema
216+
/** A schema was really supplied (see `schemaProvided`). */
217+
hasSchema: boolean
218+
}
219+
220+
/**
221+
* The SQL and schema for an operation that matches one against the other: the schema
222+
* with its identifier case folded and the SQL's quoted all-uppercase identifiers folded
223+
* to meet it (#1333) — both halves or neither, since a folded schema against unfolded
224+
* SQL, or the reverse, is the mismatch the fold exists to remove. A `schema_path`
225+
* in JSON or YAML goes through the same normalisation as an inline context; a DDL file
226+
* carries its own case semantics and is loaded as-is, with the SQL left alone.
227+
*/
228+
export function prepareSql(sql: string, schemaPath?: string, schemaContext?: Record<string, any>): PreparedSql {
229+
if (schemaPath) {
230+
const loaded = loadSchemaFile(schemaPath)
231+
if (loaded.folded) return { sql: foldQuotedIdentifierCase(sql), schema: loaded.schema, hasSchema: true }
232+
return { sql, schema: loaded.schema, hasSchema: true }
233+
}
234+
if (schemaProvided(undefined, schemaContext)) {
235+
return {
236+
sql: foldQuotedIdentifierCase(sql),
237+
schema: Schema.fromJson(normalizeSchemaContext(schemaContext!, { fold: true })),
238+
hasSchema: true,
239+
}
240+
}
241+
return { sql, schema: EMPTY_SCHEMA(), hasSchema: false }
242+
}
243+
244+
function loadSchemaFile(schemaPath: string): { schema: Schema; folded: boolean } {
245+
const ext = path.extname(schemaPath).toLowerCase()
246+
if (ext === ".json" || ext === ".yaml" || ext === ".yml") {
247+
const text = fs.readFileSync(schemaPath, "utf8")
248+
const parsed = ext === ".json" ? JSON.parse(text) : YAML.parse(text)
249+
if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) {
250+
return { schema: Schema.fromJson(normalizeSchemaContext(parsed, { fold: true })), folded: true }
251+
}
252+
}
253+
return { schema: Schema.fromFile(schemaPath), folded: false }
254+
}
255+
256+
const EMPTY_SCHEMA = () => Schema.fromDdl("CREATE TABLE _empty_ (id INT);")
257+
194258
/**
195259
* Resolve a Schema, falling back to a minimal empty schema when none is provided.
196260
* Use this for functions that require a non-null Schema argument.
@@ -201,7 +265,7 @@ export function schemaOrEmpty(
201265
): Schema {
202266
const s = resolveSchema(schemaPath, schemaContext)
203267
if (s !== null) return s
204-
return Schema.fromDdl("CREATE TABLE _empty_ (id INT);")
268+
return EMPTY_SCHEMA()
205269
}
206270

207271
export * as SchemaResolver from "./schema-resolver"

0 commit comments

Comments
 (0)