From 7d74cec45a6774b821bf47af73442c17012b853f Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 22 Jul 2026 03:27:24 +0530 Subject: [PATCH 1/2] feat(review): [R20 S1] grain-key `not_null` completeness detector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For every column named in a `dbt_utils.unique_combination_of_columns` test's `combination_of_columns`, require `not_null` coverage on the same model. Otherwise a NULL grain-key value silently passes the uniqueness test — the guardrail is toothless. Cited as a hard rule in `docs/DBT_GUIDELINES.md` in the corpus repo; kilo catches it, we didn't. Coverage sources: - `constraints: [{type: not_null}]` — only counted when `contract.enforced == true` (per codex R20 S1 high #3). On views / non-contracted models, `constraints:` is documentation-only, not enforced, so we don't miss a real gap. - Column-level `data_tests: [not_null]` / `tests: [not_null]` — always counted; dbt's test runner enforces regardless of contract state. Recommendation flips between `constraints:` (contracted model) and `data_tests:` (view / non-contracted) based on the model's contract state — matches the adapter-semantics discussion in the corpus study (PR D×2 + PR A×2). Supported YAML shapes: - `unique_combination_of_columns` and `dbt_utils.unique_combination_of_columns` (exact match per codex high #1; `endsWith` would over-match). - pre-1.9 flat args + dbt 1.9+ `arguments:` nesting. - top-level `contract:` and nested `config.contract:` for enforcement flag. False-positive guards: - Column-name comparison is case-folded (Snowflake identifiers, per codex high #2) so `WORKSPACE_ID` in `combination_of_columns` matches `workspace_id` in `columns:`. - Skipped on `status === "deleted"` files (no current grain to guard). - Only runs on files in the PR diff, not repo-wide. ### Validation on 5-PR internal corpus (recall improvement) vs S4 baseline (the tier-promotion PR this branch stacks on): | Variant | S4 baseline | S1 result | Delta | |---|---:|---:|---:| | A1 (`--pure --no-ai=true`) | 118 | 129 | **+11 grain-key gaps** | | A2 (`--pure`, LLM lane) | 143 | 150 | +7 (net; LLM run-to-run noise) | Grain-key gaps caught: - PR B (#1111) ×1: `mrt_all_purpose_auto_tune_recommendation.rec_type` - PR C (#862) ×10: workspace_id x3, job_id x2, period_start_time x2, period_end_time x2, task_key x1 - PR A, D, E: 0 (fixes already landed in HEAD state per corpus study) Strong matches to human blocker findings on PR C: - `int_job_run_billing.workspace_id`, `int_job_run_cost_carrier.workspace_id`, `int_job_task_run_cost_carrier.workspace_id` → PR C human F11 (critical: "workspace_id missing from carrier identity") - `mrt_job_run_timeline.period_end_time`, `mrt_job_task_run_timeline.period_end_time` → PR C human F10 (critical: "dbt mart grain includes period_end_time") ### Tests - 71 pass / 0 fail in `review-dbt-patterns.test.ts` (58 pre-existing + 13 new S1 tests). - 3797 pass / 640 skip / 0 fail in the full altimate review suite (132 files). - Codex-reviewed diff, 3 highs addressed: - endsWith → exact-name match - column-name case-folding - constraints only count when contract enforced - Codex minor #4 addressed (test fixture for top-level `contract:`) - Codex minor #5 addressed (test fixture for non-contracted constraints ≠ coverage) Depends on PR #1028 (feat/review-r20-s4-triage-promotion), which in turn stacks on PR #1027 (feat/review-r18-observability-recall). Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of --- .../src/altimate/review/dbt-patterns.ts | 213 ++++++++++- .../test/altimate/review-dbt-patterns.test.ts | 330 ++++++++++++++++++ 2 files changed, 542 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/altimate/review/dbt-patterns.ts b/packages/opencode/src/altimate/review/dbt-patterns.ts index eea47b289..2d5079efa 100644 --- a/packages/opencode/src/altimate/review/dbt-patterns.ts +++ b/packages/opencode/src/altimate/review/dbt-patterns.ts @@ -922,6 +922,162 @@ 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 + + // Small local helper: pull the string test-name from a YAML test entry. + // Bare form: `- not_null`, block form: `- not_null: {config: {severity: warn}}`. + const testName = (t: unknown): string | undefined => { + if (typeof t === "string") return t.toLowerCase() + if (t && typeof t === "object") { + const k = Object.keys(t as Record)[0] + return k ? k.toLowerCase() : 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). + const cfg = mm.config && typeof mm.config === "object" ? (mm.config as Record) : {} + const contractCfg = + cfg.contract && typeof cfg.contract === "object" + ? (cfg.contract as Record) + : mm.contract && typeof mm.contract === "object" + ? (mm.contract as Record) + : {} + const contractEnforced = contractCfg.enforced === true + + // Normalize 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:`. Original spelling is preserved for display in findings + // (populated below from the grain-combo entry). + const norm = (s: string): string => s.toLowerCase() + + // 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. + const coveredByConstraint = new Set() + const coveredByTest = new Set() + 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 + if (typeof type === "string" && type.toLowerCase() === "not_null") { + 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 +1141,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 +1227,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 +1268,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 +1388,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..f05cf4061 100644 --- a/packages/opencode/test/altimate/review-dbt-patterns.test.ts +++ b/packages/opencode/test/altimate/review-dbt-patterns.test.ts @@ -783,6 +783,336 @@ 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: 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( From 5ad0e852743c73b844c99ee3cd44a3fcaae2fb7e Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 22 Jul 2026 19:20:33 +0530 Subject: [PATCH 2/2] fix(review): [R20 S1] address consensus-review findings on grain-key detector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundle addresses PR #1029's consensus review: - MAJOR #1 — model-level and column-level primary_key / not_null constraints now count as coverage. dbt 1.5+ supports model-level constraints via `constraints: [{type: primary_key, columns: [a, b]}]`, and a primary_key inherently enforces NOT NULL on Postgres/Snowflake/ BigQuery/Databricks. Grain columns declared via a model-level PK were previously falsely flagged as missing not_null. Fix scans both `mm.constraints` (model-level, honouring the `columns:` list) and column-level `constraints: [{type: primary_key}]`. - MINOR #3 — contract-precedence bug. Earlier ternary short-circuited when `cfg.contract` was any object (e.g. `config: {contract: {alias: X}}` with no `enforced` key), masking a top-level `contract: {enforced: true}`. Now evaluated independently at both locations and OR'd. - MINOR #4 — `dbt.` prefix on namespaced test names (`dbt.not_null` in dbt 1.8+) is stripped in `testName()` before matching, so it counts as coverage. - MINOR #6 — `{name: , test_name: not_null}` alternative object form is recognised. Reading `Object.keys(t)[0]` returned `name` (the alias) rather than the underlying test type. Now `test_name` wins when present, falling back to the first key. - NIT #7 — `norm()` hoisted from per-model to function scope. Consensus items NOT addressed this round: - NIT #8 (dedup GrainKeyGap for a column listed twice / multiple grain tests) — collapses downstream via the global finding fingerprint; cosmetic rather than correctness. - NIT #9 (model-level `data_tests:` scanned as a grain-test source) — reviewer noted "harmless (no false match)"; no action. - NIT #10 (documentation of fallback-skip) — no code change needed. - MINOR #5 (contract resolved from dbt_project.yml or SQL config) — cross-file / cross-context, deferred. Regression tests (all pass; 82 pass / 0 fail in review-dbt-patterns suite, 3828 pass / 0 fail in full altimate suite — up from 3785): - Model-level primary_key constraint covers grain columns - Model-level not_null constraint with `columns:` list covers named cols - Column-level primary_key counts as coverage - Precision guard: PK missing cols still flagged - Model-level constraints on non-contracted model don't count - `config.contract` without `enforced` does NOT mask top-level `contract: {enforced: true}` (MINOR #3) - `dbt.not_null` covers (MINOR #4) - `{name:, test_name: not_null}` alternative form covers (MINOR #6) Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of --- .../src/altimate/review/dbt-patterns.ts | 89 +++++-- .../test/altimate/review-dbt-patterns.test.ts | 223 ++++++++++++++++++ 2 files changed, 290 insertions(+), 22 deletions(-) diff --git a/packages/opencode/src/altimate/review/dbt-patterns.ts b/packages/opencode/src/altimate/review/dbt-patterns.ts index 2d5079efa..2bfe00b75 100644 --- a/packages/opencode/src/altimate/review/dbt-patterns.ts +++ b/packages/opencode/src/altimate/review/dbt-patterns.ts @@ -957,13 +957,30 @@ function extractGrainKeyGaps(doc: unknown): GrainKeyGap[] { const d = doc as Record if (!Array.isArray(d.models)) return gaps - // Small local helper: pull the string test-name from a YAML test entry. - // Bare form: `- not_null`, block form: `- not_null: {config: {severity: warn}}`. + // 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() + if (typeof t === "string") return t.toLowerCase().replace(/^dbt\./, "") if (t && typeof t === "object") { - const k = Object.keys(t as Record)[0] - return k ? k.toLowerCase() : undefined + 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 } @@ -975,23 +992,18 @@ function extractGrainKeyGaps(doc: unknown): GrainKeyGap[] { if (!mname) continue // Contract enforcement: either at model-level `config.contract.enforced` - // OR at top-level `contract:` (dbt supports both shapes). + // 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 contractCfg = - cfg.contract && typeof cfg.contract === "object" - ? (cfg.contract as Record) - : mm.contract && typeof mm.contract === "object" - ? (mm.contract as Record) - : {} - const contractEnforced = contractCfg.enforced === true - - // Normalize 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:`. Original spelling is preserved for display in findings - // (populated below from the grain-combo entry). - const norm = (s: string): string => s.toLowerCase() + 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`. @@ -1000,8 +1012,38 @@ function extractGrainKeyGaps(doc: unknown): GrainKeyGap[] { // 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 @@ -1013,7 +1055,10 @@ function extractGrainKeyGaps(doc: unknown): GrainKeyGap[] { for (const cn of c.constraints) { if (cn && typeof cn === "object") { const type = (cn as Record).type - if (typeof type === "string" && type.toLowerCase() === "not_null") { + 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) } } diff --git a/packages/opencode/test/altimate/review-dbt-patterns.test.ts b/packages/opencode/test/altimate/review-dbt-patterns.test.ts index f05cf4061..9b7739761 100644 --- a/packages/opencode/test/altimate/review-dbt-patterns.test.ts +++ b/packages/opencode/test/altimate/review-dbt-patterns.test.ts @@ -1050,6 +1050,229 @@ models: 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: [...]}}`