review: schema.yml test-removal detector misses removals when a sibling column still has the same test

[sahrizvi]

Bug

detectSchemaYmlPatterns in packages/opencode/src/altimate/review/dbt-patterns.ts compares removed and added - unique / - not_null / - relationships test lines as raw trimmed strings across the whole diff. Any sibling column in the same file that still declares the same test type contributes an added line under the diff's re-serialization, which globally cancels the genuine removal from a different column.

Result: dropping the unique and not_null tests from one primary-key column produces zero findings if any other column in the same schema.yml still has those tests defined.

Reproduce

Given a models/schema.yml where two columns (say customer_id on customers and order_id on orders) both declare - unique and - not_null. Remove those tests from customer_id only, leaving order_id's tests intact. Run altimate-code review on the diff.

Expected: at least one finding for the removed guardrails on customers.customer_id.

Actual: zero findings. Verdict is APPROVE at trivial tier.

The trigger is that YAML re-serialization emits - unique / - not_null lines for order_id in the diff's added set, and the detector's global-string dedup treats the customer_id removals as "moved" (still present in the added set).

Root cause

Current implementation in dbt-patterns.ts:

const removedTests = removed.filter(l => /^\s*-\s*(unique|not_null|relationships)\b/.test(l))
const addedTests = new Set(added.map(l => l.trim()))
const genuinelyRemoved = removedTests.filter(l => !addedTests.has(l.trim()))

The dedup key is the test-line string alone — the detector can't tell "unique still on customer_id" apart from "unique still on order_id".

Impact

Removing guardrail tests from a primary key is a real production bug class (silent duplicate rows, null PKs, downstream aggregation double-counts). The detector silently fails on any schema.yml with more than one column declaring the same test type. Reviewers relying on the CLI to catch this pattern get a clean APPROVE/trivial/0-findings verdict on a change that removed the guardrails.

Suggested fix

Walk the unified diff with model/column context tracking (nearest preceding - name: X header at the appropriate indent depth, resetting on hunk boundaries). Key removals by (model, column, test). Only cancel a removal when a re-add appears on the same tuple. Emit one finding per removed (model, column, test) triple, with severity elevated to warning for unique and for not_null on id/key columns.

Longer-term: structural YAML AST diff of old vs new file content would be more durable than a unified-diff context walker (which can miss removals when a hunk lacks both the model and column context lines).

PR forthcoming.

[sahrizvi]

Resolved in v0.9.3 — published to npm as @altimateai/altimate-code@0.9.3 (2026-07-23).