Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions packages/opencode/src/altimate/native/altimate-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,13 @@ export function registerAll(): void {
register("altimate_core.equivalence", async (params) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW · code-reviewer] The PR introduces a deliberate use of || undefined instead of ?? undefined to coerce empty strings to undefined for dialect handling; this may conflict with CLAUDE.md if it mandates ?? universally.

💡 Suggestion: Verify that CLAUDE.md allows exceptions for dialect coercion logic. If CLAUDE.md requires ?? everywhere, add a comment referencing this PR's rationale to justify the exception.

Confidence: 80/100

try {
const schema = schemaOrEmpty(params.schema_path, params.schema_context)
const raw = await core.checkEquivalence(params.sql1, params.sql2, schema)
// Pass the optional dialect hint so dialect-specific compiled warehouse SQL
// (e.g. Snowflake semi-structured `col:field`) parses and the pair is
// decidable instead of abstaining on a syntax error. Supported since
// altimate-core@0.5.1. Use `|| undefined` (not `??`) so the default empty
// string from ReviewConfig.dialect coerces to "no hint": the engine throws
// on an unknown dialect "", and "" must mean auto-detect, not a real dialect.
const raw = await core.checkEquivalence(params.sql1, params.sql2, schema, params.dialect || undefined)
const data = toData(raw)
return ok(true, data)
} catch (e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW · code-reviewer] Inconsistent null-coalescing behavior: params.dialect || undefined is used for equivalence but params.dialect ?? undefined is used for Schema.fromDdl and other handlers.

💡 Suggestion: Standardize on ?? undefined across all handlers for consistent empty-string coercion behavior, or document why || is intentionally different for equivalence only.

Confidence: 85/100

Expand All @@ -332,7 +338,7 @@ export function registerAll(): void {
register("altimate_core.migration", async (params) => {
try {
// Build schema from old_ddl, analyze new_ddl against it
const schema = core.Schema.fromDdl(params.old_ddl, params.dialect || undefined)
const schema = core.Schema.fromDdl(params.old_ddl, params.dialect ?? undefined)
const raw = core.analyzeMigration(params.new_ddl, schema)
const data = toData(raw)
return ok(true, data)
Expand Down Expand Up @@ -426,7 +432,7 @@ export function registerAll(): void {
register("altimate_core.column_lineage", async (params) => {
try {
const schema = resolveSchema(params.schema_path, params.schema_context)
const raw = core.columnLineage(params.sql, params.dialect || undefined, schema ?? undefined)
const raw = core.columnLineage(params.sql, params.dialect ?? undefined, schema ?? undefined)
return ok(true, toData(raw))
} catch (e) {
return fail(e)
Expand All @@ -447,7 +453,7 @@ export function registerAll(): void {
// 22. altimate_core.format
register("altimate_core.format", async (params) => {
try {
const raw = core.formatSql(params.sql, params.dialect || undefined)
const raw = core.formatSql(params.sql, params.dialect ?? undefined)
const data = toData(raw)
return ok(true, data)
} catch (e) {
Expand All @@ -458,7 +464,7 @@ export function registerAll(): void {
// 23. altimate_core.metadata
register("altimate_core.metadata", async (params) => {
try {
const raw = core.extractMetadata(params.sql, params.dialect || undefined)
const raw = core.extractMetadata(params.sql, params.dialect ?? undefined)
return ok(true, toData(raw))
} catch (e) {
return fail(e)
Expand All @@ -468,7 +474,7 @@ export function registerAll(): void {
// 24. altimate_core.compare
register("altimate_core.compare", async (params) => {
try {
const raw = core.compareQueries(params.left_sql, params.right_sql, params.dialect || undefined)
const raw = core.compareQueries(params.left_sql, params.right_sql, params.dialect ?? undefined)
return ok(true, toData(raw))
} catch (e) {
return fail(e)
Expand Down Expand Up @@ -522,7 +528,7 @@ export function registerAll(): void {
// 29. altimate_core.import_ddl — returns Schema, must serialize
register("altimate_core.import_ddl", async (params) => {
try {
const schema = core.importDdl(params.ddl, params.dialect || undefined)
const schema = core.importDdl(params.ddl, params.dialect ?? undefined)
const jsonObj = schema.toJson()
return ok(true, { success: true, schema: toData(jsonObj) })
} catch (e) {
Expand Down
4 changes: 3 additions & 1 deletion packages/opencode/src/altimate/native/sql/register.ts
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,9 @@ export function registerAllSql(): void {
const sqlA = params.original ?? params.sql_a
const sqlB = params.modified ?? params.sql_b

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

// Simple line-based diff
Expand Down
2 changes: 2 additions & 0 deletions packages/opencode/src/altimate/native/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,8 @@ export interface SqlDiffParams {
original: string
modified: string
context_lines?: number
/** Optional parsing-dialect hint forwarded to the equivalence engine. */
dialect?: string
}

export interface SqlDiffResult {
Expand Down
8 changes: 6 additions & 2 deletions packages/opencode/src/altimate/review/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,14 +481,18 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun
})
const data = (res.data ?? {}) as Record<string, any>
const validationErrors = asArray(data.validation_errors)
// Undecidable when: call failed, no schema to resolve columns, or the
// engine returned validation errors. Never guess equivalent=true.
// Undecidable when: call failed, no schema to resolve columns, the engine
// returned validation errors, or the engine itself flagged the comparison
// as not decidable (altimate-core@0.5.1 `decidable` field — authoritative,
// catches semantic abstentions that carry no validation error). Never
// guess equivalent=true.
const decided =
res.success === true &&
typeof data.equivalent === "boolean" &&
!res.error &&
!data.error &&
validationErrors.length === 0 &&
data.decidable !== false &&
!!schema
if (!decided) return { decided: false }
// The engine flags column-reorder etc. as `minor` but still sets
Expand Down
98 changes: 98 additions & 0 deletions packages/opencode/test/altimate/altimate-core-native.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -816,3 +816,101 @@ describe("Real-world equivalence regressions", () => {
expect(await structRules("select distinct a from t group by a", "select distinct a from t group by a")).toEqual([])
})
})

// ---------------------------------------------------------------------------
// altimate-core@0.5.1: dialect forwarding + authoritative `decidable` field
// ---------------------------------------------------------------------------
//
// 0.5.1 added an optional `dialect` arg to checkEquivalence and a `decidable`
// flag on the result. The native equivalence handler now forwards `params.dialect`
// to the engine so dialect-specific compiled warehouse SQL parses instead of
// abstaining on a syntax error. These tests exercise the REAL engine through the
// Dispatcher (no mocks) to prove the arg is wired end-to-end.
describe("core 0.5.1 dialect forwarding + decidable", () => {
beforeAll(async () => {
registerAll()
// sql.diff lives in the sql/* handler set, registered separately.
const { registerAllSql } = await import("../../src/altimate/native/sql/register")
registerAllSql()
})

const variantSchema = {
tables: { t: { columns: [
{ name: "x", type: "INT" }, { name: "payload", type: "VARIANT" },
] } },
}
// Snowflake semi-structured access `payload:f` is a hard PARSE error in the
// default dialect (the ':' is rejected) but parses under the snowflake dialect.
// The validation-error TEXT therefore differs by dialect — a syntax error only
// when the hint is dropped. Pre-fix (dialect not forwarded) BOTH would be syntax
// errors; this asserts they diverge, proving the arg reaches the parser.
const colonSql = "select payload:f::int as v from t"
const equivCall = (dialect?: string) =>
Dispatcher.call("altimate_core.equivalence", { sql1: colonSql, sql2: colonSql, schema_context: variantSchema, dialect })
const hasSyntaxError = (data: any) =>
(data.validation_errors ?? []).some((e: string) => /Syntax error|Expected end of statement/i.test(String(e)))

test("dialect hint is forwarded to the engine (changes parse outcome)", async () => {
const noDialect = await equivCall(undefined)
const snowflake = await equivCall("snowflake")
expect(noDialect.success).toBe(true)
expect(snowflake.success).toBe(true)
// Without a dialect the colon is a syntax error; snowflake accepts it.
expect(hasSyntaxError(noDialect.data)).toBe(true)
expect(hasSyntaxError(snowflake.data)).toBe(false)
})

test("decidable=false is surfaced when the engine cannot parse/plan", async () => {
const noDialect = await equivCall(undefined)
expect((noDialect.data as any).decidable).toBe(false)
})

test("decidable=true is surfaced for a cleanly decided pair", async () => {
const r = await Dispatcher.call("altimate_core.equivalence", {
sql1: "select count(*) from t",
sql2: "select count(1) from t",
schema_context: variantSchema,
})
expect(r.success).toBe(true)
const d = r.data as any
expect(d.decidable).toBe(true)
expect(d.equivalent).toBe(true)
})

test("empty-string dialect is coerced to no-hint (does NOT throw)", async () => {
// ReviewConfig.dialect defaults to "" (auto-detect). The engine throws on an
// unknown dialect "", so the handler must coerce "" → undefined (`|| undefined`,
// not `??`). Regression guard: before the coercion this returned success=false
// and silently disabled equivalence checking on the default config.
const r = await Dispatcher.call("altimate_core.equivalence", {
sql1: "select count(*) from t",
sql2: "select count(1) from t",
schema_context: variantSchema,
dialect: "",
})
expect(r.success).toBe(true)
const d = r.data as any
expect(d.decidable).toBe(true)
expect(d.equivalent).toBe(true)
})

test("sql.diff accepts and forwards the dialect hint without throwing", async () => {
// sql.diff's runtime shape (success/diff/equivalent) predates its declared
// SqlDiffResult type, so read through `any`. The assertion that matters: the
// dialect param is plumbed into the handler's checkEquivalence call (Edit in
// sql/register.ts) and the snowflake path is reachable end-to-end.
const call = (dialect?: string) =>
Dispatcher.call("sql.diff", {
original: colonSql,
modified: colonSql,
schema_context: variantSchema,
dialect,
} as any) as Promise<any>
const noDialect = await call(undefined)
const snowflake = await call("snowflake")
expect(noDialect.success).toBe(true)
expect(snowflake.success).toBe(true)
expect(typeof noDialect.diff).toBe("string")
expect(typeof snowflake.diff).toBe("string")
})
})
143 changes: 143 additions & 0 deletions packages/opencode/test/altimate/review-equivalence-e2e.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
import { afterAll, beforeAll, describe, expect, test } from "bun:test"
import { writeFileSync } from "node:fs"
import path from "node:path"
import { runReview, DEFAULT_RUBRIC, DEFAULT_REVIEW_CONFIG, type ChangedFile } from "../../src/altimate/review"
import { createDispatcherRunner } from "../../src/altimate/review/runner"
import { registerAll } from "../../src/altimate/native/altimate-core"
import { tmpdir } from "../fixture/fixture"

// ---------------------------------------------------------------------------
// Full-stack E2E: dbt PR review pipeline → real Dispatcher → real altimate-core
// 0.5.1 engine. NO mocks. Exercises the complete chain that the dialect wiring
// (core 0.5.1 `dialect` arg) and `decidable` handling run through:
//
// runReview → semanticChangeLane → runner.equivalence(old, new, dialect)
// → Dispatcher → altimate_core.equivalence handler
// → core.checkEquivalence(sqlA, sqlB, schema, dialect)
//
// A real dbt manifest supplies the schema so the engine can resolve columns and
// actually DECIDE equivalence rather than abstain.
// ---------------------------------------------------------------------------

describe("E2E: review pipeline + real engine equivalence (core 0.5.1)", () => {
beforeAll(async () => {
process.env.ALTIMATE_TELEMETRY_DISABLED = "true"
// The DispatcherRunner calls the REAL native handlers; register them in case
// another file reset the Dispatcher.
registerAll()
const { registerAllSql } = await import("../../src/altimate/native/sql/register")
registerAllSql()
})
afterAll(() => {
delete process.env.ALTIMATE_TELEMETRY_DISABLED
})

// A real manifest with typed columns so resolveSchema() yields a usable schema.
async function reviewerWithManifest() {
const tmp = await tmpdir()
const manifestPath = path.join(tmp.path, "manifest.json")
writeFileSync(
manifestPath,
JSON.stringify({
metadata: { adapter_type: "snowflake" },
nodes: {
"model.demo.orders": {
resource_type: "model",
name: "orders",
original_file_path: "models/marts/orders.sql",
config: { materialized: "table" },
depends_on: { nodes: [] },
columns: {
id: { name: "id", data_type: "integer" },
amount: { name: "amount", data_type: "integer" },
status: { name: "status", data_type: "varchar" },
},
},
},
sources: {},
}),
)
return { tmp, runner: createDispatcherRunner({ manifestPath }) }
}

async function review(oldSql: string, newSql: string, dialect = "snowflake") {
const { tmp, runner } = await reviewerWithManifest()
try {
const files: ChangedFile[] = [{ path: "models/marts/orders.sql", status: "modified", diff: "+changed" }]
const env = await runReview({
changedFiles: files,
config: { ...DEFAULT_REVIEW_CONFIG, reviewers: ["semantic_change"], dialect, ai: false },
rubric: DEFAULT_RUBRIC,
mode: "comment",
runner,
getContent: async (_f, side) => (side === "new" ? newSql : oldSql),
getCompiled: async (_f, side) => (side === "new" ? newSql : oldSql),
generatedAt: "2026-06-10T00:00:00Z",
})
return env
} finally {
await tmp[Symbol.asyncDispose]?.()
}
}

const eqFindings = (env: Awaited<ReturnType<typeof review>>) =>
env.findings.filter((f) => f.evidence?.tool === "altimate_core.equivalence")

test("real engine DECIDES a non-equivalent rewrite through the full pipeline", async () => {
// `amount > 5` vs `amount > 6` is a genuine row-changing predicate change.
const env = await review(
"select id from orders where amount > 5",
"select id from orders where amount > 6",
)
const findings = eqFindings(env)
expect(findings.length).toBeGreaterThan(0)
const f = findings[0]
expect(f.category).toBe("semantic_change")
expect((f.evidence?.result as any)?.equivalent).toBe(false)
// The engine names the concrete predicate difference (not a vague abstention).
const diffs = (f.evidence?.result as any)?.differences ?? []
expect(diffs.length).toBeGreaterThan(0)
})

test("real engine PROVES an equivalent refactor → lane stays silent (no false positive)", async () => {
// AND-conjunct reorder is provably equivalent; the reviewer must not nitpick it.
const env = await review(
"select id from orders where amount > 5 and status = 'x'",
"select id from orders where status = 'x' and amount > 5",
)
expect(eqFindings(env)).toEqual([])
expect(env.verdict).toBe("APPROVE")
})

test("identical compiled SQL → equivalence lane is skipped entirely", async () => {
const sql = "select id from orders where amount > 5"
const env = await review(sql, sql)
expect(eqFindings(env)).toEqual([])
expect(env.verdict).toBe("APPROVE")
})

test("column projection change (drop a column) is decided NOT equivalent", async () => {
const env = await review(
"select id, amount from orders",
"select id from orders",
)
const findings = eqFindings(env)
expect(findings.length).toBeGreaterThan(0)
expect((findings[0].evidence?.result as any)?.equivalent).toBe(false)
})

test("DEFAULT config dialect (empty string) still DECIDES — no engine throw", async () => {
// ReviewConfig.dialect defaults to "". The engine throws on an unknown dialect
// "", so without coercion the lane would abstain on EVERY change under the
// default config. This drives the full pipeline with dialect="" and asserts
// the non-equivalent change is still caught (decided), proving the coercion.
const env = await review(
"select id from orders where amount > 5",
"select id from orders where amount > 6",
"", // <- default ReviewConfig.dialect
)
const findings = eqFindings(env)
expect(findings.length).toBeGreaterThan(0)
expect((findings[0].evidence?.result as any)?.equivalent).toBe(false)
})
})
Loading
Loading