Repository navigation
#260 — domain: enforcement-reality annotation + backing-test join (Slice 3) - #339
Conversation
…ce 3) The enforcement-reality surface (epic #260): a DECLARED primary-key / unique constraint that the warehouse does not ENFORCE (metadata-only on the adapter) and has no backing data test. Governance-gated; extends the `grain.unique-key-unbacked` precedent. INVESTIGATION findings (verified against source): - `metadata.adapter_type` was NOT parsed by cute-dbt — added it to `ManifestMetadata` (HEADER field, `"duckdb"` in both fixtures; skip-if-None → byte-stable). - The adapter × constraint matrix mirrors fusion's `get_constraint_support` (dbt-adapter/src/adapter/adapter_impl.rs:2328 @ dbt-labs/dbt-core main, 2026-06-13). The source CORRECTS two assumptions: Postgres does NOT enforce PK/Unique (only NotNull + FK); DuckDB "follows Postgres". - The constraint→test edge is INFERRED (the manifest never links them): a `test.*` node `attached_node`'d to the model (the proven linkage, not depends_on) with `test_metadata.name ∈ {unique, not_null}` + a column match. The copy admits this can miss renamed/custom tests. - domain (governance.rs): `enum ConstraintSupport { Enforced, NotEnforced, NotSupported }`; `constraint_support(adapter, kind)` (flat lookup, CRAP 2 — verbatim fusion matrix, unknown adapter → NotEnforced "declared, enforcement unknown", never a false claim); `backing_test_for(manifest, model, column, kind)` (the inferred join, CRAP 2). - domain (checks.rs): a new `enforcement` heuristic group (`enforcement.constraint-unbacked`, Total/DataTest) emitting per-model Findings via the EXISTING findings surface; detector decomposed (declared_unique_constraints + model/column collectors + enforcement_finding, all < CRAP 15). Copy says "declared" (not "enforced") + "backing test" (authoring discipline, never warehouse truth). - cli: the `enforcement` group is gated behind `Experiment::Governance` — `build_check_policy` drops it from `displayed` when governance is off (the detector still evaluates, per the suppression-hierarchy invariant). Byte-identity: governance-off (jaffle/playground) filters the group out → byte-identical (verified). diff-showcase (`experimental:"1"`) gains a COVERED enforcement annotation on fct_encounters (declared PK on duckdb [metadata-only], backing unique test present) → regenerated (finding + check_spec only; no root_path leak). Heuristics ledger (registry.toml + book page + SUMMARY) regenerated and byte-gated. Tests: matrix unit tests (one per adapter×kind cell, all 6 adapters + case-insensitivity + unknown-degrades); backing_test_for join tests (present / missing / renamed-test-missed-hedge / column_name-kwarg / wrong-model / disabled-test / non-unique-kinds / not_null); detector tests (fires-unbacked-PK / silent-when-backed / silent-when-enforced / no-adapter / no-constraint / column-level / check-skipped); headless guard (the enforcement finding renders on the committed showcase, naming the adapter + backing-test reality). Gates: fmt; clippy --all-targets --locked -D warnings; nextest (1823); coverage 98.61% (>=85); crap4rs PASS; bdd (28); doc -D warnings; deny; heuristics ledger byte-identical; zero-egress on committed examples; all goldens byte-identical or diff-showcase-regenerated-only. Part of #260 (Slice 3) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 13 minutes and 39 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements a new Changesenforcement.constraint-unbacked Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready to review this PR? Stage has broken it down into 6 individual chapters for you: 
Chapters generated by Stage for commit ca6a24c on Jun 13, 2026 11:03am UTC. |
📄 Rendered report previewAll golden examples regenerated cleanly. 🟡 Golden examplesCommitted to
🐶 Live dogfood previewThis PR doesn't touch 🧭 Explore previewThe two-page 🟡 Golden exploreThe committed
🐶 Live exploreThis PR doesn't touch ▶ Open ↗ opens the report or explorer in your browser in one The Pages preview may take ~1 min to update after this comment Alternative: GitHub CLI# gh CLI >= 2.63 extracts into ./report-preview-playground/.
gh run download 27464925285 -R breezy-bays-labs/cute-dbt -n report-preview-playground
open report-preview-playground/playground-report.htmlPosted by |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the enforcement.constraint-unbacked check, which is gated under the Governance experiment. It introduces an adapter-by-constraint support matrix and infers constraint-to-test edges to detect declared primary key or unique constraints that lack backing data tests on metadata-only adapters. The changes span the CLI policy builder, domain checks, manifest metadata parsing, and comprehensive unit and integration tests. The review feedback suggests optimizing the helper functions in src/domain/checks.rs by returning lazy iterators yielding &str instead of allocating temporary Strings early, deferring allocations until after deduplication.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/manifest.rs (1)
1224-1283: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd serde round-trip tests for
adapter_type.The new
adapter_typefield lacks the required serde round-trip test coverage. Following the established pattern frommanifest_metadata_project_name_round_trips_and_tolerates_absence(lines 2983-3001), add a test that verifies:
- Round-trip serialization with a populated value (e.g.,
"duckdb")- Deserialization from pre-#260 JSON (key absent) defaults to
None- Empty-string wire value is filtered to
Noneby the accessor- Explicit
nullis toleratedAs per coding guidelines, property tests for JSON serde round-trip are required for domain types.
🧪 Suggested test implementation
Add this test after line 3001:
#[test] fn manifest_metadata_adapter_type_round_trips_and_tolerates_absence() { let m = ManifestMetadata::new("v12").with_adapter_type(Some("duckdb".to_owned())); let back: ManifestMetadata = serde_json::from_str(&serde_json::to_string(&m).unwrap()).unwrap(); assert_eq!(back, m); assert_eq!(back.adapter_type(), Some("duckdb")); // Pre-#260 JSON (no adapter_type key) still deserializes. let back: ManifestMetadata = serde_json::from_str(r#"{ "dbt_schema_version": "v12" }"#).unwrap(); assert_eq!(back.adapter_type(), None); // Empty-string unset shape is filtered to None by accessor. let back: ManifestMetadata = serde_json::from_str(r#"{ "dbt_schema_version": "v12", "adapter_type": "" }"#).unwrap(); assert_eq!(back.adapter_type(), None); // Explicit null is tolerated. let back: ManifestMetadata = serde_json::from_str(r#"{ "dbt_schema_version": "v12", "adapter_type": null }"#) .unwrap(); assert_eq!(back.adapter_type(), None); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/manifest.rs` around lines 1224 - 1283, Add a serde round-trip unit test for ManifestMetadata that mirrors the existing project_name test: create a ManifestMetadata::new("v12").with_adapter_type(Some("duckdb".to_owned())), serialize and deserialize and assert equality and adapter_type() == Some("duckdb"); verify deserializing pre-#260 JSON without the adapter_type key yields adapter_type() == None; verify deserializing with "adapter_type": "" yields adapter_type() == None (empty-string filtered by adapter_type()); and verify "adapter_type": null is tolerated and yields None. Place the test alongside the other ManifestMetadata serde tests and name it manifest_metadata_adapter_type_round_trips_and_tolerates_absence.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domain/checks.rs`:
- Around line 1084-1094: The code is emitting synthetic attribution strings like
"{kind_label}[{column}]" into Verdict::Covered.by even though backing_test_for
only returns a boolean; change this to use real test node ids: update
backing_test_for to return the matching test id(s) (e.g.,
Option<Vec<TestNodeId>> or Vec<TestNodeId>) or add a new helper that looks up
the manifest tests for ctx.model.id(), column and kind and returns their node
ids, then populate Verdict::Covered.by with those actual ids instead of
format!("{kind_label}[{column}]"); if no backing test ids are found, do not emit
synthetic attribution (leave by empty or omit the Covered verdict) and keep
Evidence::new("backing-test", ...) but reflect presence/absence based on the
real lookup; references: backing_test_for, kind_tag, Verdict::Covered,
Evidence::new, ctx.manifest, ctx.model.id().
- Around line 1026-1034: The function model_level_unique_columns currently
flattens composite model-level constraints into per-column entries, which yields
incorrect single-column findings; change model_level_unique_columns to preserve
tuple-level constraints by either (a) changing its return type to
Vec<(Vec<String>, ConstraintKind)> and mapping each constraint to
(c.columns().clone(), kind) so multi-column constraints remain grouped, or (b)
if you prefer the minimal change now, filter out multi-column constraints and
only return single-column constraints (e.g., keep entries where
c.columns().len() == 1) to avoid producing false per-column findings; update any
callers of model_level_unique_columns to handle the new grouped form or to
accept that multi-column constraints are excluded, and ensure
unique_constraint_kind, c.columns(), and Node usages remain consistent with the
new signature/behavior.
---
Outside diff comments:
In `@src/domain/manifest.rs`:
- Around line 1224-1283: Add a serde round-trip unit test for ManifestMetadata
that mirrors the existing project_name test: create a
ManifestMetadata::new("v12").with_adapter_type(Some("duckdb".to_owned())),
serialize and deserialize and assert equality and adapter_type() ==
Some("duckdb"); verify deserializing pre-#260 JSON without the adapter_type key
yields adapter_type() == None; verify deserializing with "adapter_type": ""
yields adapter_type() == None (empty-string filtered by adapter_type()); and
verify "adapter_type": null is tolerated and yields None. Place the test
alongside the other ManifestMetadata serde tests and name it
manifest_metadata_adapter_type_round_trips_and_tolerates_absence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd5c89a8-ec9f-43de-869e-dd11308d313d
📒 Files selected for processing (11)
book/src/SUMMARY.mdbook/src/checks/enforcement.constraint-unbacked.mdbook/src/checks/index.mdexamples/diff-showcase-report.htmlheuristics/registry.tomlsrc/cli/mod.rssrc/domain/checks.rssrc/domain/governance.rssrc/domain/manifest.rssrc/domain/mod.rstests/headless_toggle.rs
… (CI red on #339) Two CI failures on #339 (main is green) — both fixed: 1. EXPLORE GOLDEN LEAK (the missed arm): the new `enforcement` heuristic leaked into the gate-free `explore` page. `explore` renders findings via `build_payload` → `CheckPolicy::default()`, which displayed EVERY registered check (incl. the governance-gated `enforcement` group). So the experimental enforcement finding surfaced on fct_encounters in the non-experimental explore `tests.html` (+2485 bytes). My Slice 3 gate lived only in the report's `build_check_policy` — explore + the `[checks]`-config arm both bypassed it. Fix (the gating posture, not a golden regen): an experiment-gated check is now OFF in `CheckPolicy::default()`. Added `CheckId::is_experimental` (default-impl, keyed on `EXPERIMENT_GATED_GROUPS = ["enforcement"]`, so synthetic test registries are unaffected); `default()` filters experimental checks out. The report's `build_check_policy` now RECONCILES (was: remove-when-off): governance ON re-adds the gated checks in registry order, governance OFF removes them — the single authority over BOTH policy arms. Result: explore (gate-free) + non-governance report never surface enforcement; the governance-on diff-showcase still does. All goldens byte-identical (report arm AND explore arm — verified both; no regen needed). 2. CLIPPY (CI red, cached local --locked passed): `cargo clean` reproduced 4 errors in `constraint_support` — `match_same_arms` (the per-adapter matrix rows have identical bodies across adapters) + a `doc_markdown` missing-backticks. Allowed `match_same_arms` WITH justification (the rows are a deliberate verbatim mirror of fusion's table — merging collapses the audit trail) and backticked the doc. Gates re-run COMPLETE this time (both arms): fmt; clippy --all-targets --locked -D warnings on a CLEAN build (exit 0); nextest (1823); coverage 98.61% (>=85); crap4rs PASS; bdd (28); doc -D warnings; deny; heuristics ledger byte-identical; REPORT goldens (jaffle/playground/diff-showcase) AND EXPLORE golden (dag.html + tests.html) byte-identical; governance + enforcement + explore-zero-egress headless. Part of #260 (Slice 3) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-gate #339) Consolidated pass for the 3 unresolved threads + CI clippy on #339: 1. CI CLIPPY (the merge blocker — stale local lint): two `clippy::items-after-statements` in `src/cli/mod.rs` test fns (the `use crate::domain::CheckId as _;` placed after `let` statements). Hoisted each to the top of its fn. `cargo clean && clippy --all-targets --locked` now passes. 2. CodeRabbit MAJOR (checks.rs attribution): `backing_test_for` returned `bool` and the detector synthesized a fake `Verdict::Covered.by` (`"unique[col]"`) — unlike every other Covered.by in the registry, which carries a REAL node id. Changed `backing_test_for` to return `Option<&NodeId>` (the matching test node id, via `.find().map()`); the detector now attributes the real id (e.g. `test.healthcare_analytics.unique_fct_encounters_encounter_key.…`) and drops the synthetic label. The diff-showcase golden's enforcement Covered.by is regenerated to the real id (finding payload only; no root_path leak). backing_test_for stays CRAP 2. 3. CodeRabbit MAJOR (checks.rs composite flattening): a COMPOSITE model-level PK/unique was flattened to N per-column "unbacked" findings — a FALSE claim (a composite key asserts the TUPLE is unique, backed by unique_combination_of_columns, not per-column). Filter composite model-level constraints OUT (`columns().len() == 1`) — silence over falsehood, the check's never-a-false-claim invariant. Single-column model-level + all column-level constraints still fire. Full composite-aware inference filed as cute-dbt#341 (the `// tracked:` marker is at the filter site). 4. gemini MEDIUM (column_level_unique_columns temp Vec): N (columns per model) is small; the Vec feeds a chain. Negligible — not changed (see thread reply). Tests: backing_test_for now returns/asserts the real node id (present / the missing / renamed-hedge / disabled / wrong-model / kwarg-column / non-unique-None cases); the detector's Covered.by asserts the real test id; composite-key emits no per-column finding (cute-dbt#341) while a single-column model-level key still fires. Gates (COMPLETE, both golden arms): fmt; clippy --all-targets --locked -D warnings on a CLEAN build (exit 0); nextest (1825); coverage 98.61% (>=85); crap4rs PASS (backing_test_for 2, all touched fns <15); bdd (28); doc -D warnings; deny; heuristics ledger byte-identical; REPORT goldens (jaffle/playground/diff-showcase) AND EXPLORE golden (dag.html + tests.html) byte-identical; governance + enforcement + explore-zero-egress headless. Part of #260 (Slice 3) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Part of #260 (Slice 3). The enforcement-reality surface — the only fully-verbatim-copy surface, extending the
grain.unique-key-unbackedprecedent. A DECLARED primary-key / unique constraint that the warehouse does NOT enforce (metadata-only on the adapter) and has no backing data test.Investigation findings (verified against source — AGENTS.md rule)
metadata.adapter_typewas NOT parsed by cute-dbt. The brief said to read it; it didn't exist. Added it toManifestMetadata(a HEADER field,"duckdb"in both committed fixtures;skip_serializing_if→ byte-stable).get_constraint_support(crates/dbt-adapter/src/adapter/adapter_impl.rs:2328@dbt-labs/dbt-coremain, verified 2026-06-13). The source corrects two assumptions in the brief: Postgres does not enforce PK/Unique (only NotNull + FK); DuckDB "follows Postgres." Athena/Spark/Trino areunimplemented!in fusion (not NotSupported) → I map unknown/unimplemented adapters toNotEnforced("declared, enforcement unknown" — never a falseEnforced).test.*nodeattached_node'd to the model — the proven linkage the grain check uses, notdepends_on.nodes— withtest_metadata.name ∈ {unique, not_null}+ a column match (column_namekwarg, node-level fallback). The copy admits this can miss renamed/custom tests.What this adds
governance.rs):enum ConstraintSupport { Enforced, NotEnforced, NotSupported };constraint_support(adapter, kind)(flat lookup, CRAP 2 — verbatim fusion matrix);backing_test_for(manifest, model, column, kind)(the inferred join, CRAP 2).checks.rs): a newenforcementheuristic group (enforcement.constraint-unbacked, Total/DataTest) emitting per-modelFindings via the existing findings surface (no render changes). Detector decomposed (declared_unique_constraints+ model/column collectors +enforcement_finding, all < CRAP 15).enforcementgroup is gated behindExperiment::Governance—build_check_policydrops it fromdisplayedwhen governance is off (the detector still evaluates, per the suppression-hierarchy invariant; it just never displays).Copy (verbatim primary + required hedging)
Says "declared" not "enforced" (the matrix qualifies it: "metadata-only on {adapter}"), and "backing test: {present|missing}" as authoring discipline — never implies the warehouse lacks an index (columns are authored-YAML-only; the inferred edge can miss renamed/custom tests).
Byte-identity
jaffle/playground): theenforcementgroup is filtered out of the policy → byte-identical (verified).diff-showcase(experimental:"1"): gains a COVERED enforcement annotation onfct_encounters(declared PKencounter_keyon duckdb [metadata-only], backing unique test present). Regenerated (finding + check_spec only; noroot_pathleak).registry.toml+ the new book check page +SUMMARY.md) regenerated and byte-gated bytests/heuristics_ledger.rs.Tests
backing_test_forjoin tests: present / missing / renamed-test-missed (the hedge) /column_name-kwarg / wrong-model / disabled-test / non-unique-kinds / not_null.Gates (all green, run directly)
--all-targets --locked -D warnings✓ (exit 0) · nextest ✓ (1823 passed, 101 ignored)constraint_support2,backing_test_for2, detector helpers ≤3 — all < 15)-D warnings✓ · deny ✓ · heuristics ledger byte-identical ✓ · zero-egress on committed examples ✓🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
enforcement.constraint-unbackedcheck to detect models declaring primary key/unique constraints without corresponding data tests backing them on the given adapter.Documentation