diff --git a/packages/opencode/src/altimate/review/dbt-patterns.ts b/packages/opencode/src/altimate/review/dbt-patterns.ts index eea47b289..2bfe00b75 100644 --- a/packages/opencode/src/altimate/review/dbt-patterns.ts +++ b/packages/opencode/src/altimate/review/dbt-patterns.ts @@ -922,6 +922,207 @@ function extractTestOccurrences(doc: unknown): Set { return out } +/** + * R20 S1 — grain-key `not_null` completeness. + * + * A `dbt_utils.unique_combination_of_columns` test names N columns as the + * declared grain. If any grain column lacks `not_null` coverage, a NULL + * grain-key silently passes the uniqueness test — the guardrail is toothless. + * `DBT_GUIDELINES.md` in the corpus repo states this as a hard rule; kilo + * catches it, we didn't. + * + * Coverage sources (either counts): + * - `constraints: [{type: not_null}]` on the column — enforced by the + * database when the model has `contract: {enforced: true}` on Trino / + * Databricks-with-contracts / Postgres / Snowflake. + * - `data_tests: [not_null]` / `tests: [not_null]` on the column — + * enforced by dbt test runner regardless of contract. + * + * Returns one gap per uncovered column per grain declaration. + */ +export interface GrainKeyGap { + /** Model / entity name. */ + model: string + /** Column name in the uncovered `combination_of_columns`. */ + column: string + /** True when `config.contract.enforced == true` — coverage should be a + * `constraints:` entry rather than a `data_tests:` entry, since the + * constraint enforces at write-time and the test enforces at CI-time. */ + contractEnforced: boolean +} + +function extractGrainKeyGaps(doc: unknown): GrainKeyGap[] { + const gaps: GrainKeyGap[] = [] + if (!doc || typeof doc !== "object") return gaps + const d = doc as Record + if (!Array.isArray(d.models)) return gaps + + // Normalise column names for coverage comparison. dbt YAML often uses + // adapter-cased column names (Snowflake folds unquoted identifiers to + // uppercase; other adapters differ). Lowercase both sides so `WORKSPACE_ID` + // in `combination_of_columns` matches `workspace_id` in `columns:`. Hoisted + // to function scope per consensus NIT #7 (was redeclared per-model). + const norm = (s: string): string => s.toLowerCase() + + // Pull the string test-name from a YAML test entry. Consensus fixes: + // - MINOR #6: dbt's documented alternative object form is + // `{name: , test_name: not_null, ...}`. `Object.keys(t)[0]` + // would return `name`, missing the underlying test type. Prefer + // `test_name` when present. + // - MINOR #4: dbt 1.8+ allows namespaced names like `dbt.not_null`; + // strip a leading `dbt.` prefix so `dbt.not_null` matches `not_null`. + // Bare form: `- not_null`. Block form: `- not_null: {config: ...}`. + const testName = (t: unknown): string | undefined => { + if (typeof t === "string") return t.toLowerCase().replace(/^dbt\./, "") + if (t && typeof t === "object") { + const obj = t as Record + // Alternative form: `{name: , test_name: , ...}`. + // If both `name` and `test_name` are present, `test_name` wins. + if (typeof obj.test_name === "string") return obj.test_name.toLowerCase().replace(/^dbt\./, "") + const k = Object.keys(obj)[0] + return k ? k.toLowerCase().replace(/^dbt\./, "") : undefined + } + return undefined + } + + for (const m of d.models) { + if (!m || typeof m !== "object") continue + const mm = m as Record + const mname = typeof mm.name === "string" ? mm.name : undefined + if (!mname) continue + + // Contract enforcement: either at model-level `config.contract.enforced` + // OR at top-level `contract:` (dbt supports both shapes). Consensus + // MINOR #3 — earlier ternary short-circuited on `cfg.contract` being + // an object (e.g. `config: {contract: {alias: ...}}` with no + // `enforced` key), masking a top-level `contract: {enforced: true}`. + // Evaluate `enforced === true` at both locations independently and OR + // them so either declaration counts. + const cfg = mm.config && typeof mm.config === "object" ? (mm.config as Record) : {} + const cfgContract = + cfg.contract && typeof cfg.contract === "object" ? (cfg.contract as Record) : undefined + const topContract = + mm.contract && typeof mm.contract === "object" ? (mm.contract as Record) : undefined + const contractEnforced = cfgContract?.enforced === true || topContract?.enforced === true + + // Coverage per column, split by mechanism (per codex R20 S1 review): + // - constraint coverage counts ONLY when `contract.enforced == true`. + // On non-contracted models, `constraints: [{type: not_null}]` is + // documentation-only and not enforced by the database. Requiring + // contract-enforced means we don't miss a real gap on views. + // - test coverage (`tests:` / `data_tests: [not_null]`) always counts, + // since dbt's test runner enforces it independent of contract state. + // + // Consensus MAJOR #1 — also count MODEL-LEVEL constraints: + // - `constraints: [{type: primary_key, columns: [a, b]}]` (dbt 1.5+) + // inherently enforces NOT NULL on every named column on + // Postgres/Snowflake/BigQuery/Databricks, so grain columns declared + // via a model-level PK are already covered. + // - `constraints: [{type: not_null, columns: [...]}]` (dbt's model- + // level form) — same coverage, just spelled out. + // - Column-level `constraints: [{type: primary_key}]` — same rationale + // at column granularity. + const coveredByConstraint = new Set() + const coveredByTest = new Set() + + // Model-level constraints — dbt allows a `constraints:` list under + // the model itself (not per-column) that names one or more columns. + if (Array.isArray(mm.constraints)) { + for (const cn of mm.constraints) { + if (!cn || typeof cn !== "object") continue + const cnRec = cn as Record + const type = typeof cnRec.type === "string" ? cnRec.type.toLowerCase() : "" + // `primary_key` implies NOT NULL on every listed column across the + // adapters dbt supports for enforced contracts; `not_null` at + // model level is the explicit multi-column variant of the column + // form. + if (type !== "not_null" && type !== "primary_key") continue + const cols = Array.isArray(cnRec.columns) ? (cnRec.columns as unknown[]) : [] + for (const c of cols) { + if (typeof c === "string") coveredByConstraint.add(norm(c)) + } + } + } + + if (Array.isArray(mm.columns)) { + for (const col of mm.columns) { + if (!col || typeof col !== "object") continue + const c = col as Record + const cname = typeof c.name === "string" ? c.name : undefined + if (!cname) continue + const key = norm(cname) + if (Array.isArray(c.constraints)) { + for (const cn of c.constraints) { + if (cn && typeof cn === "object") { + const type = (cn as Record).type + const t = typeof type === "string" ? type.toLowerCase() : "" + // Column-level `not_null` OR `primary_key` — the latter + // inherently enforces NOT NULL (consensus MAJOR #1). + if (t === "not_null" || t === "primary_key") { + coveredByConstraint.add(key) + } + } + } + } + for (const testsKey of ["tests", "data_tests"] as const) { + const tests = c[testsKey] + if (!Array.isArray(tests)) continue + for (const t of tests) { + if (testName(t) === "not_null") coveredByTest.add(key) + } + } + } + } + const hasCoverage = (col: string): boolean => { + const k = norm(col) + return coveredByTest.has(k) || (contractEnforced && coveredByConstraint.has(k)) + } + + // Find grain declarations in this model's model-level tests / data_tests. + // Restrict to `unique_combination_of_columns` and + // `dbt_utils.unique_combination_of_columns` exactly (per codex R20 S1 + // review) — `endsWith` would over-match `not_unique_combination_of_columns` + // and third-party macros that share the suffix. + // Supports both dbt shapes: + // pre-1.9: `- dbt_utils.unique_combination_of_columns: {combination_of_columns: [...]}` + // 1.9+: `- dbt_utils.unique_combination_of_columns: {arguments: {combination_of_columns: [...]}}` + const isGrainTestName = (name: string): boolean => { + const n = name.toLowerCase() + return n === "unique_combination_of_columns" || n === "dbt_utils.unique_combination_of_columns" + } + for (const testsKey of ["tests", "data_tests"] as const) { + const tests = mm[testsKey] + if (!Array.isArray(tests)) continue + for (const t of tests) { + if (!t || typeof t !== "object") continue + const entry = t as Record + for (const k of Object.keys(entry)) { + if (!isGrainTestName(k)) continue + const args = entry[k] + if (!args || typeof args !== "object") continue + const argsObj = args as Record + const nested = + argsObj.arguments && typeof argsObj.arguments === "object" + ? (argsObj.arguments as Record) + : undefined + const combo = + (nested?.combination_of_columns as unknown) ?? (argsObj.combination_of_columns as unknown) + if (!Array.isArray(combo)) continue + for (const col of combo) { + if (typeof col !== "string") continue + if (!hasCoverage(col)) { + // Preserve original grain-column spelling in the finding. + gaps.push({ model: mname, column: col, contractEnforced }) + } + } + } + } + } + } + + return gaps +} + /** * Fallback removed-test detector for callers that supply only the diff * (no old/new content). Kept intentionally conservative: matches removed @@ -985,6 +1186,8 @@ export function detectSchemaYmlPatterns( testTag?: string }> = [] let usedStructural = false + // R20 S1 — populated from the structural NEW-side parse below. + let grainGaps: GrainKeyGap[] = [] // Deleting a whole schema.yml removes every test declared in it — arguably // a bigger removal than dropping a single test. Treat the new side as empty @@ -1069,6 +1272,16 @@ export function detectSchemaYmlPatterns( removals.push({ model, column, test, testTag }) } usedStructural = true + // R20 S1 — grain-key `not_null` completeness. Only fire on + // non-deleted files: a deleted schema.yml has no current grain to + // guard. Extract from the NEW side so we flag the current state of + // the file, whether the grain declaration was newly added in this + // PR or pre-existing (a broken grain-guard is a real risk either + // way, and reviewers can suppress if the pre-existing case is by + // design). Uses the `not_null-missing` gap emitted below. + if (!isDeletedFile && newDoc !== undefined) { + grainGaps = extractGrainKeyGaps(newDoc) + } } } @@ -1100,7 +1313,7 @@ export function detectSchemaYmlPatterns( } } - if (!removals.length) return [] + if (!removals.length && !grainGaps.length) return [] const findings: Finding[] = [] const isMartLayer = /(^|\/)(marts?|reporting)\//.test(file.path) @@ -1220,5 +1433,48 @@ export function detectSchemaYmlPatterns( }), ) } + + // R20 S1 — grain-key `not_null` completeness. For every column in a + // `unique_combination_of_columns` test's `combination_of_columns` that + // lacks `not_null` coverage on the same model, emit one finding. + // Recommendation flips between `constraints:` (contracted model) and + // `data_tests:` (view / non-contracted model) based on the model's + // contract state — matches the adapter-semantics discussion in the + // corpus study (PR D×2, PR A×2). + for (const g of grainGaps) { + const filename = file.path.split("/").pop() + const recommendation = g.contractEnforced + ? `Add \`constraints: [{type: not_null}]\` to \`${g.column}\` on \`${g.model}\` (contract is \`enforced: true\` so the constraint is enforced at write-time).` + : `Add \`not_null\` to \`${g.column}\`'s \`data_tests:\` on \`${g.model}\` (contract is not enforced, so a \`constraints:\` entry would be inert — use a data_test).` + findings.push( + makeFinding({ + severity: clampSeverity("test_coverage", "warning", "high"), + category: "test_coverage", + title: `${filename}: grain column \`${g.column}\` in unique_combination_of_columns lacks \`not_null\` on \`${g.model}\``, + body: + `The \`unique_combination_of_columns\` test on \`${g.model}\` names \`${g.column}\` as a grain key, but no \`not_null\` ` + + `coverage is declared for it (either as a \`constraints:\` entry on a contracted model, or as a \`data_tests: [not_null]\` ` + + `on a view). A NULL grain-key value silently passes the uniqueness test, so a fan-out or duplicate bug can ship without ` + + `any test catching it. ${recommendation}`, + file: file.path, + model: g.model, + column: g.column, + confidence: "high", + evidence: { + tool: "dbt-patterns", + result: { + rule: "grain_key_not_null_missing", + model: g.model, + column: g.column, + contractEnforced: g.contractEnforced, + }, + }, + // Per-column ruleKey so the global fingerprint dedupe keeps distinct + // grain-column gaps on the same model as separate findings. + ruleKey: `test_coverage:grain-key-not-null:${g.model}.${g.column}`, + }), + ) + } + return findings.filter((x) => !exclusionReason(x, rubric)) } diff --git a/packages/opencode/test/altimate/review-dbt-patterns.test.ts b/packages/opencode/test/altimate/review-dbt-patterns.test.ts index 509586309..9b7739761 100644 --- a/packages/opencode/test/altimate/review-dbt-patterns.test.ts +++ b/packages/opencode/test/altimate/review-dbt-patterns.test.ts @@ -783,6 +783,559 @@ models: [] expect(f.length).toBe(0) }) + // R20 S1 — grain-key `not_null` completeness. Every column named in a + // `unique_combination_of_columns` test's `combination_of_columns` must have + // `not_null` coverage on the same model (constraint if contracted, data_test + // if view). Directly targets PR D×2 + PR A×2 human findings in the R20 + // corpus study; explicit rule in DBT_GUIDELINES.md. + test("R20 S1: unique_combination_of_columns with grain col missing not_null → warning finding", () => { + // `price_start_time` is grain but has no not_null coverage → gap. + // `metastore_id` / `sku_name` have not_null via constraints on the + // contracted model → covered. + const newContent = `version: 2 +models: + - name: mrt_billing_account_prices + config: + contract: + enforced: true + columns: + - name: metastore_id + constraints: + - type: not_null + - name: sku_name + constraints: + - type: not_null + - name: price_start_time + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: + - metastore_id + - sku_name + - price_start_time +` + const oldContent = newContent // steady-state (grain already present) — still fires + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_billing_account_prices.yml", status: "modified", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent, newContent }, + ) + const gap = f.find((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing") + expect(gap).toBeDefined() + expect(gap!.severity).toBe("warning") + expect(gap!.model).toBe("mrt_billing_account_prices") + expect(gap!.column).toBe("price_start_time") + // Contract is enforced → recommendation should point at `constraints:`. + expect(gap!.body).toContain("constraints: [{type: not_null}]") + // Non-gap columns must not appear as findings. + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(1) + }) + + test("R20 S1: non-contracted (view) model recommends data_tests: not_null", () => { + const newContent = `version: 2 +models: + - name: stg_billing + columns: + - name: metastore_id + data_tests: + - not_null + - name: sku_name + data_tests: + - not_null + - name: price_start_time # ← no not_null on this grain col + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: + - metastore_id + - sku_name + - price_start_time +` + const f = detectSchemaYmlPatterns( + { path: "models/staging/stg_billing.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + const gap = f.find((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing") + expect(gap).toBeDefined() + // Non-contracted model → recommendation should point at `data_tests:`. + expect(gap!.body).toContain("data_tests:") + expect(gap!.body).not.toContain("constraints: [{type: not_null}]") + }) + + test("R20 S1: grain col covered by column-level tests: [not_null] (dbt <1.8 alias) is not a gap", () => { + const newContent = `version: 2 +models: + - name: mrt_x + columns: + - name: id + tests: + - not_null + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: [id] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + + test("R20 S1: SCD2-style grain (change_time) missing not_null flagged (PR D F4 shape)", () => { + const newContent = `version: 2 +models: + - name: mrt_job_tasks_inventory + config: + contract: + enforced: true + columns: + - name: metastore_id + constraints: [{type: not_null}] + - name: job_id + constraints: [{type: not_null}] + - name: task_key + constraints: [{type: not_null}] + - name: change_time # ← temporal grain, no not_null + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: + - metastore_id + - job_id + - task_key + - change_time +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_job_tasks_inventory.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + const gaps = f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing") + expect(gaps.length).toBe(1) + expect(gaps[0].column).toBe("change_time") + }) + + test("R20 S1: no false positive when every grain col has not_null coverage", () => { + const newContent = `version: 2 +models: + - name: mrt_x + columns: + - name: a + data_tests: [not_null] + - name: b + data_tests: [not_null] + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: [a, b] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + + test("R20 S1: bare unique_combination_of_columns (no `dbt_utils.` prefix) also matches", () => { + const newContent = `version: 2 +models: + - name: mrt_x + columns: + - name: id + data_tests: + - unique_combination_of_columns: + combination_of_columns: [id] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(1) + }) + + test("R20 S1: non-contracted model — `constraints: [not_null]` does NOT count as coverage", () => { + // On a view / non-contracted model, `constraints:` is documentation + // only (not enforced by the DB). Only column-level `not_null` data_tests + // should count as coverage. Codex R20 S1 high #3. + const newContent = `version: 2 +models: + - name: stg_x # ← no config.contract.enforced + columns: + - name: id + constraints: + - type: not_null # ← doesn't count on non-contracted model + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: [id] +` + const f = detectSchemaYmlPatterns( + { path: "models/staging/stg_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + const gaps = f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing") + expect(gaps.length).toBe(1) + expect(gaps[0].body).toContain("data_tests:") + }) + + test("R20 S1: top-level `contract: {enforced: true}` (not nested under config:) is recognised", () => { + // dbt supports declaring contract enforcement either at model.config.contract + // or at model.contract directly. Both must count as contract-enforced so + // the recommendation correctly suggests `constraints:`. + const newContent = `version: 2 +models: + - name: mrt_x + contract: + enforced: true + columns: + - name: id + constraints: [{type: not_null}] + - name: change_time # ← grain col, no not_null + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: [id, change_time] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + const gaps = f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing") + expect(gaps.length).toBe(1) + expect(gaps[0].column).toBe("change_time") + // Contract IS enforced (via top-level `contract:`) → recommendation should + // point at `constraints:`, not data_tests. + expect(gaps[0].body).toContain("constraints: [{type: not_null}]") + }) + + test("R20 S1: test-name match is exact, not endsWith (false-positive guard)", () => { + // `not_unique_combination_of_columns` (fictional but plausible) or a + // third-party macro ending in the same suffix must NOT trigger the rule. + const newContent = `version: 2 +models: + - name: mrt_x + columns: + - name: id + data_tests: + - some_package.not_unique_combination_of_columns: + combination_of_columns: [id] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + + test("R20 S1: adapter case-folding — SNOWFLAKE_ID grain col matches snowflake_id column coverage", () => { + // Snowflake folds unquoted identifiers to uppercase. If someone writes + // `combination_of_columns: [WORKSPACE_ID]` while the column is declared + // as `- name: workspace_id`, the coverage should still match. + const newContent = `version: 2 +models: + - name: mrt_x + columns: + - name: workspace_id + data_tests: [not_null] + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: [WORKSPACE_ID] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + + test("R20 S1: MODEL-level primary_key constraint covers every listed grain column (consensus MAJOR #1)", () => { + // Consensus MAJOR #1 — dbt 1.5+ supports model-level constraints via + // `constraints: [{type: primary_key, columns: [a, b]}]`. A primary key + // inherently enforces NOT NULL on Postgres/Snowflake/BigQuery/ + // Databricks. Grain columns declared via a model-level PK constraint + // must not be flagged as missing not_null. + const newContent = `version: 2 +models: + - name: mrt_x + config: + contract: + enforced: true + constraints: + - type: primary_key + columns: [metastore_id, sku_name, price_start_time] + columns: + - name: metastore_id + - name: sku_name + - name: price_start_time + - name: currency + data_tests: + - dbt_utils.unique_combination_of_columns: + arguments: + combination_of_columns: [metastore_id, sku_name, price_start_time] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + + test("R20 S1: MODEL-level not_null constraint with `columns:` list covers each named column", () => { + // Explicit multi-column form of the model-level constraint. Same + // coverage effect as the primary_key case. + const newContent = `version: 2 +models: + - name: mrt_x + config: + contract: + enforced: true + constraints: + - type: not_null + columns: [a, b] + columns: + - name: a + - name: b + - name: c + data_tests: + - dbt_utils.unique_combination_of_columns: + arguments: + combination_of_columns: [a, b] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + + test("R20 S1: COLUMN-level primary_key constraint also counts as not_null coverage", () => { + const newContent = `version: 2 +models: + - name: mrt_x + config: + contract: + enforced: true + columns: + - name: id + constraints: + - type: primary_key + data_tests: + - dbt_utils.unique_combination_of_columns: + arguments: + combination_of_columns: [id] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + + test("R20 S1: model-level primary_key still misses columns NOT in its list", () => { + // Precision guard — a PK that names only some grain cols must still + // leave the OTHER grain cols flagged. + const newContent = `version: 2 +models: + - name: mrt_x + config: + contract: + enforced: true + constraints: + - type: primary_key + columns: [a] # only covers a + columns: + - name: a + - name: b + data_tests: + - dbt_utils.unique_combination_of_columns: + arguments: + combination_of_columns: [a, b] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + const gaps = f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing") + expect(gaps.length).toBe(1) + expect(gaps[0].column).toBe("b") + }) + + test("R20 S1: model-level constraints on NON-CONTRACTED model do NOT count as coverage (documentation-only)", () => { + // Consistent with column-level rule — model-level constraints without + // contract enforcement are documentation, not enforcement, on most + // adapters. + const newContent = `version: 2 +models: + - name: stg_x + constraints: + - type: primary_key + columns: [a, b] + columns: + - name: a + - name: b + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: [a, b] +` + const f = detectSchemaYmlPatterns( + { path: "models/staging/stg_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + const gaps = f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing") + // Both columns flagged (no test-level not_null, no contract). + expect(gaps.length).toBe(2) + }) + + test("R20 S1: `config.contract` object without `enforced` does NOT mask a top-level `contract: {enforced: true}` (MINOR #3)", () => { + // Consensus MINOR #3 — an earlier ternary short-circuited when + // `cfg.contract` was any object. `config: {contract: {alias: X}}` + // with no `enforced` key hid a top-level `contract: {enforced: true}`. + // Now both locations are OR'd; either declaration counts. + const newContent = `version: 2 +models: + - name: mrt_x + config: + contract: + alias: SOMETHING_ELSE # no enforced key here + contract: + enforced: true # ← must still count as contracted + columns: + - name: id + constraints: + - type: not_null + - name: change_time # ← grain col, no not_null + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: [id, change_time] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + const gaps = f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing") + expect(gaps.length).toBe(1) + expect(gaps[0].column).toBe("change_time") + // Contract IS enforced — recommendation must point at constraints. + expect(gaps[0].body).toContain("constraints: [{type: not_null}]") + }) + + test("R20 S1: `dbt.not_null` namespaced test alias counts as coverage (MINOR #4)", () => { + // Consensus MINOR #4 — dbt 1.8+ allows namespaced test names. Column + // covered by `data_tests: [dbt.not_null]` must not be flagged. + const newContent = `version: 2 +models: + - name: mrt_x + columns: + - name: id + data_tests: [dbt.not_null] + data_tests: + - unique_combination_of_columns: + combination_of_columns: [id] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + + test("R20 S1: `{name:, test_name: not_null}` alternative test form counts as coverage (MINOR #6)", () => { + // Consensus MINOR #6 — dbt's documented alternative object form is + // `{name: my_test, test_name: not_null, ...}`. Reading only the first + // key returns `name` (an alias), missing the underlying test type. The + // `test_name` field is authoritative when present. + const newContent = `version: 2 +models: + - name: mrt_x + columns: + - name: id + data_tests: + - name: id_never_null + test_name: not_null + data_tests: + - unique_combination_of_columns: + combination_of_columns: [id] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + + test("R20 S1: dbt 1.9+ `arguments:` nesting is recognised (real corpus shape)", () => { + // The real internal corpus PRs use the dbt 1.9+ shape: + // `- dbt_utils.unique_combination_of_columns: {arguments: {combination_of_columns: [...]}}` + // Detector must recognise both nested (`arguments:`) and pre-1.9 flat forms. + const newContent = `version: 2 +models: + - name: mrt_x + config: + contract: + enforced: true + columns: + - name: a + constraints: [{type: not_null}] + - name: b # ← missing not_null + data_tests: + - dbt_utils.unique_combination_of_columns: + arguments: + combination_of_columns: [a, b] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + const gaps = f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing") + expect(gaps.length).toBe(1) + expect(gaps[0].column).toBe("b") + }) + + test("R20 S1: does not fire when there's no unique_combination_of_columns test", () => { + const newContent = `version: 2 +models: + - name: mrt_x + columns: + - name: a + - name: b +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "added", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent: undefined, newContent }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + + test("R20 S1: does not fire on deleted schema.yml (no current grain to guard)", () => { + const oldContent = `version: 2 +models: + - name: mrt_x + columns: + - name: id + data_tests: + - dbt_utils.unique_combination_of_columns: + combination_of_columns: [id] +` + const f = detectSchemaYmlPatterns( + { path: "models/marts/mrt_x.yml", status: "deleted", diff: undefined }, + DEFAULT_RUBRIC, + { oldContent, newContent: undefined }, + ) + expect(f.filter((x) => (x.evidence?.result as any)?.rule === "grain_key_not_null_missing").length).toBe(0) + }) + test("benign additive column produces NO dbt-pattern finding (precision)", () => { const sql = `select id, upper(status) as status_upper from {{ ref('x') }}` const f = detectModelPatterns(