Skip to content

Commit 53127b7

Browse files
anandgupta42claude
andauthored
feat: wire altimate-core 0.5.1 equivalence dialect + decidable (#928)
altimate-core 0.5.1 (#925) added an optional `dialect` arg to `checkEquivalence` and a `decidable` flag on `EquivalenceResult`. The dbt PR reviewer already threads a dialect end-to-end, but commit 89f77a7 had to DROP it at the engine boundary because 0.4.0 only took 3 params. This forwards it now that 0.5.1 accepts it, and consumes `decidable`. - `native/altimate-core.ts`: forward the dialect hint to `core.checkEquivalence`. Use `|| undefined` (not `??`) so the default empty `ReviewConfig.dialect` coerces to "no hint" — the engine throws on `unknown dialect ''`, and "" must mean auto-detect. - `native/sql/register.ts`: same dialect forwarding + coercion in the `sql.diff` handler. - `native/types.ts`: declare `dialect?` on `SqlDiffParams`. - `review/runner.ts`: honor the engine's authoritative `decidable` flag (`data.decidable !== false`). Strictly safer — only ever abstains more; backward-compatible when the field is absent (0.4.0 shape). Tests (all real-engine where it counts): - `altimate-core-native.test.ts`: dialect forwarding changes parse outcome, `decidable` surfaced true/false, empty-string coercion, sql.diff. - `review-equivalence-e2e.test.ts`: full `runReview` pipeline → real engine — non-equivalent caught, equivalent stays silent, identical skipped, projection change caught, and the default empty-string-dialect path still decides (regression guard). - `review-runner.test.ts`: runner honors decidable=false / true / absent. Verified: `bun run typecheck` clean, marker check clean, full new-test suite green. Reviewed via multi-model consensus (Gemini independently caught the empty-string dialect throw, now fixed + guarded). The pre-existing sibling sites (`columnLineage`/`formatSql`/etc.) share the empty-string fragility but are protected by `run.ts` resolving the dialect first; tracked separately in #927. Closes #926 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent ea6a2a0 commit 53127b7

7 files changed

Lines changed: 339 additions & 10 deletions

File tree

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

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,13 @@ export function registerAll(): void {
320320
register("altimate_core.equivalence", async (params) => {
321321
try {
322322
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
323-
const raw = await core.checkEquivalence(params.sql1, params.sql2, schema)
323+
// Pass the optional dialect hint so dialect-specific compiled warehouse SQL
324+
// (e.g. Snowflake semi-structured `col:field`) parses and the pair is
325+
// decidable instead of abstaining on a syntax error. Supported since
326+
// altimate-core@0.5.1. Use `|| undefined` (not `??`) so the default empty
327+
// string from ReviewConfig.dialect coerces to "no hint": the engine throws
328+
// on an unknown dialect "", and "" must mean auto-detect, not a real dialect.
329+
const raw = await core.checkEquivalence(params.sql1, params.sql2, schema, params.dialect || undefined)
324330
const data = toData(raw)
325331
return ok(true, data)
326332
} catch (e) {
@@ -332,7 +338,7 @@ export function registerAll(): void {
332338
register("altimate_core.migration", async (params) => {
333339
try {
334340
// Build schema from old_ddl, analyze new_ddl against it
335-
const schema = core.Schema.fromDdl(params.old_ddl, params.dialect || undefined)
341+
const schema = core.Schema.fromDdl(params.old_ddl, params.dialect ?? undefined)
336342
const raw = core.analyzeMigration(params.new_ddl, schema)
337343
const data = toData(raw)
338344
return ok(true, data)
@@ -426,7 +432,7 @@ export function registerAll(): void {
426432
register("altimate_core.column_lineage", async (params) => {
427433
try {
428434
const schema = resolveSchema(params.schema_path, params.schema_context)
429-
const raw = core.columnLineage(params.sql, params.dialect || undefined, schema ?? undefined)
435+
const raw = core.columnLineage(params.sql, params.dialect ?? undefined, schema ?? undefined)
430436
return ok(true, toData(raw))
431437
} catch (e) {
432438
return fail(e)
@@ -447,7 +453,7 @@ export function registerAll(): void {
447453
// 22. altimate_core.format
448454
register("altimate_core.format", async (params) => {
449455
try {
450-
const raw = core.formatSql(params.sql, params.dialect || undefined)
456+
const raw = core.formatSql(params.sql, params.dialect ?? undefined)
451457
const data = toData(raw)
452458
return ok(true, data)
453459
} catch (e) {
@@ -458,7 +464,7 @@ export function registerAll(): void {
458464
// 23. altimate_core.metadata
459465
register("altimate_core.metadata", async (params) => {
460466
try {
461-
const raw = core.extractMetadata(params.sql, params.dialect || undefined)
467+
const raw = core.extractMetadata(params.sql, params.dialect ?? undefined)
462468
return ok(true, toData(raw))
463469
} catch (e) {
464470
return fail(e)
@@ -468,7 +474,7 @@ export function registerAll(): void {
468474
// 24. altimate_core.compare
469475
register("altimate_core.compare", async (params) => {
470476
try {
471-
const raw = core.compareQueries(params.left_sql, params.right_sql, params.dialect || undefined)
477+
const raw = core.compareQueries(params.left_sql, params.right_sql, params.dialect ?? undefined)
472478
return ok(true, toData(raw))
473479
} catch (e) {
474480
return fail(e)
@@ -522,7 +528,7 @@ export function registerAll(): void {
522528
// 29. altimate_core.import_ddl — returns Schema, must serialize
523529
register("altimate_core.import_ddl", async (params) => {
524530
try {
525-
const schema = core.importDdl(params.ddl, params.dialect || undefined)
531+
const schema = core.importDdl(params.ddl, params.dialect ?? undefined)
526532
const jsonObj = schema.toJson()
527533
return ok(true, { success: true, schema: toData(jsonObj) })
528534
} catch (e) {

packages/opencode/src/altimate/native/sql/register.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,9 @@ export function registerAllSql(): void {
359359
const sqlA = params.original ?? params.sql_a
360360
const sqlB = params.modified ?? params.sql_b
361361

362-
const compareRaw = schema ? await core.checkEquivalence(sqlA, sqlB, schema) : null
362+
// `|| undefined`: coerce a default empty-string dialect to "no hint" — the
363+
// engine throws on an unknown dialect "".
364+
const compareRaw = schema ? await core.checkEquivalence(sqlA, sqlB, schema, params.dialect || undefined) : null
363365
const compare = compareRaw ? JSON.parse(JSON.stringify(compareRaw)) : null
364366

365367
// Simple line-based diff

packages/opencode/src/altimate/native/types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -724,6 +724,8 @@ export interface SqlDiffParams {
724724
original: string
725725
modified: string
726726
context_lines?: number
727+
/** Optional parsing-dialect hint forwarded to the equivalence engine. */
728+
dialect?: string
727729
}
728730

729731
export interface SqlDiffResult {

packages/opencode/src/altimate/review/runner.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -481,14 +481,18 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun
481481
})
482482
const data = (res.data ?? {}) as Record<string, any>
483483
const validationErrors = asArray(data.validation_errors)
484-
// Undecidable when: call failed, no schema to resolve columns, or the
485-
// engine returned validation errors. Never guess equivalent=true.
484+
// Undecidable when: call failed, no schema to resolve columns, the engine
485+
// returned validation errors, or the engine itself flagged the comparison
486+
// as not decidable (altimate-core@0.5.1 `decidable` field — authoritative,
487+
// catches semantic abstentions that carry no validation error). Never
488+
// guess equivalent=true.
486489
const decided =
487490
res.success === true &&
488491
typeof data.equivalent === "boolean" &&
489492
!res.error &&
490493
!data.error &&
491494
validationErrors.length === 0 &&
495+
data.decidable !== false &&
492496
!!schema
493497
if (!decided) return { decided: false }
494498
// The engine flags column-reorder etc. as `minor` but still sets

packages/opencode/test/altimate/altimate-core-native.test.ts

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -816,3 +816,101 @@ describe("Real-world equivalence regressions", () => {
816816
expect(await structRules("select distinct a from t group by a", "select distinct a from t group by a")).toEqual([])
817817
})
818818
})
819+
820+
// ---------------------------------------------------------------------------
821+
// altimate-core@0.5.1: dialect forwarding + authoritative `decidable` field
822+
// ---------------------------------------------------------------------------
823+
//
824+
// 0.5.1 added an optional `dialect` arg to checkEquivalence and a `decidable`
825+
// flag on the result. The native equivalence handler now forwards `params.dialect`
826+
// to the engine so dialect-specific compiled warehouse SQL parses instead of
827+
// abstaining on a syntax error. These tests exercise the REAL engine through the
828+
// Dispatcher (no mocks) to prove the arg is wired end-to-end.
829+
describe("core 0.5.1 dialect forwarding + decidable", () => {
830+
beforeAll(async () => {
831+
registerAll()
832+
// sql.diff lives in the sql/* handler set, registered separately.
833+
const { registerAllSql } = await import("../../src/altimate/native/sql/register")
834+
registerAllSql()
835+
})
836+
837+
const variantSchema = {
838+
tables: { t: { columns: [
839+
{ name: "x", type: "INT" }, { name: "payload", type: "VARIANT" },
840+
] } },
841+
}
842+
// Snowflake semi-structured access `payload:f` is a hard PARSE error in the
843+
// default dialect (the ':' is rejected) but parses under the snowflake dialect.
844+
// The validation-error TEXT therefore differs by dialect — a syntax error only
845+
// when the hint is dropped. Pre-fix (dialect not forwarded) BOTH would be syntax
846+
// errors; this asserts they diverge, proving the arg reaches the parser.
847+
const colonSql = "select payload:f::int as v from t"
848+
const equivCall = (dialect?: string) =>
849+
Dispatcher.call("altimate_core.equivalence", { sql1: colonSql, sql2: colonSql, schema_context: variantSchema, dialect })
850+
const hasSyntaxError = (data: any) =>
851+
(data.validation_errors ?? []).some((e: string) => /Syntax error|Expected end of statement/i.test(String(e)))
852+
853+
test("dialect hint is forwarded to the engine (changes parse outcome)", async () => {
854+
const noDialect = await equivCall(undefined)
855+
const snowflake = await equivCall("snowflake")
856+
expect(noDialect.success).toBe(true)
857+
expect(snowflake.success).toBe(true)
858+
// Without a dialect the colon is a syntax error; snowflake accepts it.
859+
expect(hasSyntaxError(noDialect.data)).toBe(true)
860+
expect(hasSyntaxError(snowflake.data)).toBe(false)
861+
})
862+
863+
test("decidable=false is surfaced when the engine cannot parse/plan", async () => {
864+
const noDialect = await equivCall(undefined)
865+
expect((noDialect.data as any).decidable).toBe(false)
866+
})
867+
868+
test("decidable=true is surfaced for a cleanly decided pair", async () => {
869+
const r = await Dispatcher.call("altimate_core.equivalence", {
870+
sql1: "select count(*) from t",
871+
sql2: "select count(1) from t",
872+
schema_context: variantSchema,
873+
})
874+
expect(r.success).toBe(true)
875+
const d = r.data as any
876+
expect(d.decidable).toBe(true)
877+
expect(d.equivalent).toBe(true)
878+
})
879+
880+
test("empty-string dialect is coerced to no-hint (does NOT throw)", async () => {
881+
// ReviewConfig.dialect defaults to "" (auto-detect). The engine throws on an
882+
// unknown dialect "", so the handler must coerce "" → undefined (`|| undefined`,
883+
// not `??`). Regression guard: before the coercion this returned success=false
884+
// and silently disabled equivalence checking on the default config.
885+
const r = await Dispatcher.call("altimate_core.equivalence", {
886+
sql1: "select count(*) from t",
887+
sql2: "select count(1) from t",
888+
schema_context: variantSchema,
889+
dialect: "",
890+
})
891+
expect(r.success).toBe(true)
892+
const d = r.data as any
893+
expect(d.decidable).toBe(true)
894+
expect(d.equivalent).toBe(true)
895+
})
896+
897+
test("sql.diff accepts and forwards the dialect hint without throwing", async () => {
898+
// sql.diff's runtime shape (success/diff/equivalent) predates its declared
899+
// SqlDiffResult type, so read through `any`. The assertion that matters: the
900+
// dialect param is plumbed into the handler's checkEquivalence call (Edit in
901+
// sql/register.ts) and the snowflake path is reachable end-to-end.
902+
const call = (dialect?: string) =>
903+
Dispatcher.call("sql.diff", {
904+
original: colonSql,
905+
modified: colonSql,
906+
schema_context: variantSchema,
907+
dialect,
908+
} as any) as Promise<any>
909+
const noDialect = await call(undefined)
910+
const snowflake = await call("snowflake")
911+
expect(noDialect.success).toBe(true)
912+
expect(snowflake.success).toBe(true)
913+
expect(typeof noDialect.diff).toBe("string")
914+
expect(typeof snowflake.diff).toBe("string")
915+
})
916+
})
Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
import { afterAll, beforeAll, describe, expect, test } from "bun:test"
2+
import { writeFileSync } from "node:fs"
3+
import path from "node:path"
4+
import { runReview, DEFAULT_RUBRIC, DEFAULT_REVIEW_CONFIG, type ChangedFile } from "../../src/altimate/review"
5+
import { createDispatcherRunner } from "../../src/altimate/review/runner"
6+
import { registerAll } from "../../src/altimate/native/altimate-core"
7+
import { tmpdir } from "../fixture/fixture"
8+
9+
// ---------------------------------------------------------------------------
10+
// Full-stack E2E: dbt PR review pipeline → real Dispatcher → real altimate-core
11+
// 0.5.1 engine. NO mocks. Exercises the complete chain that the dialect wiring
12+
// (core 0.5.1 `dialect` arg) and `decidable` handling run through:
13+
//
14+
// runReview → semanticChangeLane → runner.equivalence(old, new, dialect)
15+
// → Dispatcher → altimate_core.equivalence handler
16+
// → core.checkEquivalence(sqlA, sqlB, schema, dialect)
17+
//
18+
// A real dbt manifest supplies the schema so the engine can resolve columns and
19+
// actually DECIDE equivalence rather than abstain.
20+
// ---------------------------------------------------------------------------
21+
22+
describe("E2E: review pipeline + real engine equivalence (core 0.5.1)", () => {
23+
beforeAll(async () => {
24+
process.env.ALTIMATE_TELEMETRY_DISABLED = "true"
25+
// The DispatcherRunner calls the REAL native handlers; register them in case
26+
// another file reset the Dispatcher.
27+
registerAll()
28+
const { registerAllSql } = await import("../../src/altimate/native/sql/register")
29+
registerAllSql()
30+
})
31+
afterAll(() => {
32+
delete process.env.ALTIMATE_TELEMETRY_DISABLED
33+
})
34+
35+
// A real manifest with typed columns so resolveSchema() yields a usable schema.
36+
async function reviewerWithManifest() {
37+
const tmp = await tmpdir()
38+
const manifestPath = path.join(tmp.path, "manifest.json")
39+
writeFileSync(
40+
manifestPath,
41+
JSON.stringify({
42+
metadata: { adapter_type: "snowflake" },
43+
nodes: {
44+
"model.demo.orders": {
45+
resource_type: "model",
46+
name: "orders",
47+
original_file_path: "models/marts/orders.sql",
48+
config: { materialized: "table" },
49+
depends_on: { nodes: [] },
50+
columns: {
51+
id: { name: "id", data_type: "integer" },
52+
amount: { name: "amount", data_type: "integer" },
53+
status: { name: "status", data_type: "varchar" },
54+
},
55+
},
56+
},
57+
sources: {},
58+
}),
59+
)
60+
return { tmp, runner: createDispatcherRunner({ manifestPath }) }
61+
}
62+
63+
async function review(oldSql: string, newSql: string, dialect = "snowflake") {
64+
const { tmp, runner } = await reviewerWithManifest()
65+
try {
66+
const files: ChangedFile[] = [{ path: "models/marts/orders.sql", status: "modified", diff: "+changed" }]
67+
const env = await runReview({
68+
changedFiles: files,
69+
config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["semantic_change"], dialect, ai: false },
70+
rubric: DEFAULT_RUBRIC,
71+
mode: "comment",
72+
runner,
73+
getContent: async (_f, side) => (side === "new" ? newSql : oldSql),
74+
getCompiled: async (_f, side) => (side === "new" ? newSql : oldSql),
75+
generatedAt: "2026-06-10T00:00:00Z",
76+
})
77+
return env
78+
} finally {
79+
await tmp[Symbol.asyncDispose]?.()
80+
}
81+
}
82+
83+
const eqFindings = (env: Awaited<ReturnType<typeof review>>) =>
84+
env.findings.filter((f) => f.evidence?.tool === "altimate_core.equivalence")
85+
86+
test("real engine DECIDES a non-equivalent rewrite through the full pipeline", async () => {
87+
// `amount > 5` vs `amount > 6` is a genuine row-changing predicate change.
88+
const env = await review(
89+
"select id from orders where amount > 5",
90+
"select id from orders where amount > 6",
91+
)
92+
const findings = eqFindings(env)
93+
expect(findings.length).toBeGreaterThan(0)
94+
const f = findings[0]
95+
expect(f.category).toBe("semantic_change")
96+
expect((f.evidence?.result as any)?.equivalent).toBe(false)
97+
// The engine names the concrete predicate difference (not a vague abstention).
98+
const diffs = (f.evidence?.result as any)?.differences ?? []
99+
expect(diffs.length).toBeGreaterThan(0)
100+
})
101+
102+
test("real engine PROVES an equivalent refactor → lane stays silent (no false positive)", async () => {
103+
// AND-conjunct reorder is provably equivalent; the reviewer must not nitpick it.
104+
const env = await review(
105+
"select id from orders where amount > 5 and status = 'x'",
106+
"select id from orders where status = 'x' and amount > 5",
107+
)
108+
expect(eqFindings(env)).toEqual([])
109+
expect(env.verdict).toBe("APPROVE")
110+
})
111+
112+
test("identical compiled SQL → equivalence lane is skipped entirely", async () => {
113+
const sql = "select id from orders where amount > 5"
114+
const env = await review(sql, sql)
115+
expect(eqFindings(env)).toEqual([])
116+
expect(env.verdict).toBe("APPROVE")
117+
})
118+
119+
test("column projection change (drop a column) is decided NOT equivalent", async () => {
120+
const env = await review(
121+
"select id, amount from orders",
122+
"select id from orders",
123+
)
124+
const findings = eqFindings(env)
125+
expect(findings.length).toBeGreaterThan(0)
126+
expect((findings[0].evidence?.result as any)?.equivalent).toBe(false)
127+
})
128+
129+
test("DEFAULT config dialect (empty string) still DECIDES — no engine throw", async () => {
130+
// ReviewConfig.dialect defaults to "". The engine throws on an unknown dialect
131+
// "", so without coercion the lane would abstain on EVERY change under the
132+
// default config. This drives the full pipeline with dialect="" and asserts
133+
// the non-equivalent change is still caught (decided), proving the coercion.
134+
const env = await review(
135+
"select id from orders where amount > 5",
136+
"select id from orders where amount > 6",
137+
"", // <- default ReviewConfig.dialect
138+
)
139+
const findings = eqFindings(env)
140+
expect(findings.length).toBeGreaterThan(0)
141+
expect((findings[0].evidence?.result as any)?.equivalent).toBe(false)
142+
})
143+
})

0 commit comments

Comments
 (0)