Repository navigation
#254 — versioned-model resolution via tested_node_unique_id - #263
Conversation
A versioned dbt model's unique_id ends in its version suffix (model.proj.dim_customers.v2), so leaf-segment bare-name matching can never bind it — covered-by attribution and every name-keyed lookup silently missed versioned models. Ingest the engine-resolved ids on the domain UnitTest (tested_node_unique_id + the this_input_node_unique_id twin; fusion dbt-schemas manifest_nodes.rs:273-274 @ 9977b6cb) and add resolve_tested_model: prefer the direct nodes lookup when the id is present, fall back to resolve_target_model bare-name matching when absent/dangling/non-model (graceful absence for dbt-core and older manifests). Switch every unit-test target resolution in the domain (scope, state, preflight, checks) to the new bridge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WireUnitTest gains tested_node_unique_id / this_input_node_unique_id (Option + serde(default): tolerant of absent keys — every committed fixture predates the fields — and explicit nulls, the engine null-fill shape). into_domain threads them via builders. The render indexer (index_tests_for_models) and both explore attribution surfaces (unit_test_counts, unit_test_paths_by_model) switch to resolve_tested_model so a versioned model's tests bind on every surface. Goldens verified byte-identical (no committed fixture uses model versions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 10 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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 (3)
📝 WalkthroughWalkthroughThe PR introduces engine-resolved unit-test target binding. ChangesEngine-resolved unit-test target binding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 5 individual chapters for you: 
Chapters generated by Stage for commit 0583dcd on Jun 12, 2026 2:12am UTC. |
📄 Rendered report previewAll golden examples regenerated cleanly. 🟡 Golden examplesCommitted to
🐶 Live dogfood previewThis PR doesn't touch ▶ Open ↗ opens the report in your browser in one click — 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 27390048228 -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 addresses an issue where unit tests targeting versioned models were not correctly resolved because bare-name matching failed on version suffixes (e.g., .v2). It introduces resolve_tested_model, which prioritizes the engine-resolved tested_node_unique_id from the manifest over bare-name matching, falling back gracefully if the ID is absent or null. The codebase has been updated to use this new resolution logic across adapters, checks, and scoping. Feedback is provided to refactor resolve_tested_model using idiomatic Option combinators instead of an explicit if let block.
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/adapters/render.rs (1)
2273-2305:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a render-level regression test for versioned unit-test indexing.
This wiring change is correct, but this file’s test suite does not pin the new
tested_node_unique_idpath. Please add a case where a unit test targetsmodel.proj.dim_customers.v2viatested_node_unique_idwhilemodel:stays bare, then assertindex_tests_for_models/build_payloadindexes it under the versionedNodeId. Without that, this adapter can silently drift back to bare-name binding.As per coding guidelines,
**/*.rs: "Implement TDD — tests before implementation for all domain and adapter code."🤖 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/adapters/render.rs` around lines 2273 - 2305, Add a render-level regression test that constructs a Manifest containing a UnitTest whose tested_node_unique_id is the versioned target (e.g., "model.proj.dim_customers.v2") while its bare `model:` reference remains unversioned, then call index_tests_for_models (and build_payload if present in this file's tests) with a ModelInScopeSet containing the versioned NodeId and assert the returned map uses the versioned NodeId key and includes the test; ensure the test exercises resolve_tested_model path that reads `tested_node_unique_id` so the indexer cannot regress back to bare-name binding.Source: Coding guidelines
src/adapters/explore.rs (1)
490-519: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winThe new engine-resolved binding is not fully regression-pinned across
src/adapters/explore.rs,src/domain/scope.rs, andsrc/domain/preflight.rs.src/adapters/explore.rsproves the count path for versioned targets, but not the paths payload;src/domain/scope.rsandsrc/domain/preflight.rsswitch toresolve_tested_modelwithout a file-local versioned-model test at all. Please add one versioned-model fixture per consumer layer so the engine-resolvedtested_node_unique_idbranch stays locked for explore attribution, PR-diff scoping, and preflight error naming.As per coding guidelines, "Implement TDD — tests before implementation for all domain and adapter code."
🤖 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/adapters/explore.rs` around lines 490 - 519, Add a unit test fixture exercising an engine-resolved versioned model for each consumer layer so the tested_node_unique_id branch is regression-pinned: (1) in tests around unit_test_paths_by_model ensure the payload includes the versioned model path (not just count) by creating a unit_test that resolves via resolve_tested_model to a versioned model and asserting the returned UnitTestPathsPayload.yaml/fixtures contain the versioned path; (2) in the domain scope tests (tests for resolve_tested_model/Scope-related behavior) add a file-local versioned-model fixture exercising the tested_node_unique_id branch and assert resolution; and (3) in preflight tests (preflight.rs) add a corresponding versioned-model fixture and assert preflight error naming uses the engine-resolved tested_node_unique_id. Reference functions/structures: unit_test_paths_by_model, resolve_tested_model, UnitTestPathsPayload, and the preflight error naming code path so each layer has one explicit versioned-model test before making further changes.Source: Coding guidelines
🧹 Nitpick comments (2)
src/domain/unit_test.rs (1)
754-799: ⚡ Quick winExhaust the new resolved-id option matrix across both the domain and adapter tests.
src/domain/unit_test.rsandsrc/adapters/manifest.rscurrently cover only the all-none/all-some corners fortested_node_unique_idandthis_input_node_unique_id. The shared root cause is sampled coverage of a 2-field optional serde contract: mixed states (Some/None,None/Some, plus absent vs explicitnullon the wire) are whereskip_serializing_if/ tolerant-deserialization regressions would hide, so both files should use the repo’s usual table-driven matrix coverage for these new fields.🤖 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/unit_test.rs` around lines 754 - 799, Add a table-driven test in src/domain/unit_test.rs that exhausts the 2x2 matrix and wire variations for tested_node_unique_id and this_input_node_unique_id (covering (None,None),(Some,None),(None,Some),(Some,Some) using bare_unit_test() plus with_tested_node_unique_id(...) and with_this_input_node_unique_id(...)); for each case assert tested_node_unique_id() / this_input_node_unique_id() values, assert serialization includes the key only when Some and omits when None, and also exercise deserialization from JSON with the key absent and with the key present as explicit null to ensure tolerant deserialization/skipping behavior (use the existing resolved_node_ids_default_none_and_skip_keys and resolved_node_ids_roundtrip_when_present as templates).Sources: Coding guidelines, Learnings
src/domain/checks.rs (1)
1150-1152: ⚡ Quick winAdd a detector regression for the versioned-model resolution path.
These are the detector-side consumers of
resolve_tested_model, but the file still does not pin the case this PR is fixing: an ambiguous baremodelname with atested_node_unique_idthat targets the correct versioned model. Without that, checks can regress back to leaf-name matching while the shared resolver still looks correct elsewhere. As per coding guidelines,**/*.rs: Implement TDD — tests before implementation for all domain and adapter code. Based on PR objectives, this PR fixes versioned-model resolution viatested_node_unique_id.Also applies to: 1569-1572
🤖 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/checks.rs` around lines 1150 - 1152, Add a detector regression test that exercises the versioned-model resolution path for resolve_tested_model: create a manifest with two versions of the same bare model name and a tested node having tested_node_unique_id that should pick the correct versioned model, then assert resolve_tested_model(ctx.manifest, ut) returns Some(model) whose id() equals ctx.model.id() (covering the ambiguous bare name + tested_node_unique_id scenario); place the test before changing domain logic per TDD, use the same test harness/setup used by existing checks in this module and target the iterator/filter case that uses resolve_tested_model so the regression cannot revert to leaf-name-only matching.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.
Outside diff comments:
In `@src/adapters/explore.rs`:
- Around line 490-519: Add a unit test fixture exercising an engine-resolved
versioned model for each consumer layer so the tested_node_unique_id branch is
regression-pinned: (1) in tests around unit_test_paths_by_model ensure the
payload includes the versioned model path (not just count) by creating a
unit_test that resolves via resolve_tested_model to a versioned model and
asserting the returned UnitTestPathsPayload.yaml/fixtures contain the versioned
path; (2) in the domain scope tests (tests for
resolve_tested_model/Scope-related behavior) add a file-local versioned-model
fixture exercising the tested_node_unique_id branch and assert resolution; and
(3) in preflight tests (preflight.rs) add a corresponding versioned-model
fixture and assert preflight error naming uses the engine-resolved
tested_node_unique_id. Reference functions/structures: unit_test_paths_by_model,
resolve_tested_model, UnitTestPathsPayload, and the preflight error naming code
path so each layer has one explicit versioned-model test before making further
changes.
In `@src/adapters/render.rs`:
- Around line 2273-2305: Add a render-level regression test that constructs a
Manifest containing a UnitTest whose tested_node_unique_id is the versioned
target (e.g., "model.proj.dim_customers.v2") while its bare `model:` reference
remains unversioned, then call index_tests_for_models (and build_payload if
present in this file's tests) with a ModelInScopeSet containing the versioned
NodeId and assert the returned map uses the versioned NodeId key and includes
the test; ensure the test exercises resolve_tested_model path that reads
`tested_node_unique_id` so the indexer cannot regress back to bare-name binding.
---
Nitpick comments:
In `@src/domain/checks.rs`:
- Around line 1150-1152: Add a detector regression test that exercises the
versioned-model resolution path for resolve_tested_model: create a manifest with
two versions of the same bare model name and a tested node having
tested_node_unique_id that should pick the correct versioned model, then assert
resolve_tested_model(ctx.manifest, ut) returns Some(model) whose id() equals
ctx.model.id() (covering the ambiguous bare name + tested_node_unique_id
scenario); place the test before changing domain logic per TDD, use the same
test harness/setup used by existing checks in this module and target the
iterator/filter case that uses resolve_tested_model so the regression cannot
revert to leaf-name-only matching.
In `@src/domain/unit_test.rs`:
- Around line 754-799: Add a table-driven test in src/domain/unit_test.rs that
exhausts the 2x2 matrix and wire variations for tested_node_unique_id and
this_input_node_unique_id (covering
(None,None),(Some,None),(None,Some),(Some,Some) using bare_unit_test() plus
with_tested_node_unique_id(...) and with_this_input_node_unique_id(...)); for
each case assert tested_node_unique_id() / this_input_node_unique_id() values,
assert serialization includes the key only when Some and omits when None, and
also exercise deserialization from JSON with the key absent and with the key
present as explicit null to ensure tolerant deserialization/skipping behavior
(use the existing resolved_node_ids_default_none_and_skip_keys and
resolved_node_ids_roundtrip_when_present as templates).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f5c89e4-44c2-4bdd-b356-db10962ff631
📒 Files selected for processing (9)
src/adapters/explore.rssrc/adapters/manifest.rssrc/adapters/render.rssrc/domain/checks.rssrc/domain/mod.rssrc/domain/preflight.rssrc/domain/scope.rssrc/domain/state.rssrc/domain/unit_test.rs
…tor chain Review feedback (gemini-code-assist on PR #263): replace the if-let early-return with a single .or_else() chain. Behavior-identical — the dangling-id and non-model fallback paths are pinned by resolve_tested_model_falls_back_when_the_id_dangles and resolve_tested_model_falls_back_when_the_id_names_a_non_model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Orchestrator wrap-up (overnight session). Merged at d9f75b2 after: gate green (33/33 checks), the one Gemini thread probe-confirmed behavior-identical and applied ( |
1 participant
Summary
Versioned dbt models (
model.proj.dim_customers.v2) mis-bound in cute-dbt's name resolution:leaf_segment()-style matching returns the version suffix ("v2") as the model name, so a versioned model's unit tests silently dropped out of covered-by attribution, scope selection, and every name-keyed lookup. The engine-resolved fix was already on the wire and dropped: this PR ingeststested_node_unique_id(and itsthis_input_node_unique_idtwin) on the unit-test node and adds a single resolution authority,domain::state::resolve_tested_model, that prefers the engine-resolved id (directnodeslookup, model-typed) and falls back to the existingresolve_target_modelbare-name match when the field is absent, null, dangling, or non-model — exact pre-#254 behavior for dbt-core and older manifests.Acceptance criteria — evidence
.v2fixture) — RED→GREEN unit tests insrc/domain/state.rs:resolve_target_model_misses_a_versioned_model_by_bare_namepins the bug shape;resolve_tested_model_binds_a_versioned_model_via_engine_resolved_id+in_scope_unit_tests_includes_a_test_on_a_modified_versioned_modelprove resolution and attribution (in-scope + models-in-scope arm 1). All in-process synthetic fixtures — no committed manifest regen, no real data.tested_node_unique_id+ the input twin ingested and used for attribution instead of string-splitting where available —WireUnitTest(adapter) andUnitTest(domain,skip_serializing_ifso pre-domain: versioned-model name resolution mis-binds (leaf_segment returns the version suffix) — ingest tested_node_unique_id #254 payloads stay byte-stable) carry both fields; all 12 unit-test target-resolution call sites switched toresolve_tested_model(scope ×3, state ×3, preflight ×1, checks ×2, renderindex_tests_for_models×1, exploreunit_test_counts+unit_test_paths_by_model×2). Wire tolerance tests cover absent keys (every committed fixture) and explicit nulls (engine null-fill — the feat: surface incremental-model unit-test semantics (incremental badge + expect=merged-rows tooltip) #145 precedent).latest_version/deprecation_datedecided — deferred to adapters: ingest governance wire family — exposures, groups/owner/access, model versions/deprecation #256 (the next slice in the epic Epic: manifest wire-extension wave — ingest the governance/contract/test-config surfaces dropped today #255 serial lane). They are model-node governance fields, not unit-test resolution fields; this slice stays mechanical on the correctness bug. Note:this_input_node_unique_idis ingested and carried but has no distinct consumer today — thethisgiven already binds through the (now-correct) target model; fusion never populates the field as of the pinned SHA.Fusion citation (SHA-pinned)
dbt-labs/dbt-fusion@9977b6cbb1b761065536300037560d8e3c037011(same pin as the #252 research):crates/dbt-schemas/src/schemas/manifest/manifest_nodes.rs:273-274—ManifestUnitTest.tested_node_unique_id/this_input_node_unique_id, bothOption<String>, unit-test nodes only (data tests carryattached_node, already ingested).crates/dbt-parser/src/resolve/resolve_tests/resolve_unit_tests.rs:120-130, 370— runtime meaning: the unversioned model's unique_id when resolvable,Nonewhen not (test goes to disabled); each per-version unit-test clone gets the.vN-suffixed versioned model unique_id.dbt-project/, dbt 2.0.0-preview.177, never committed):tested_node_unique_idpopulated on every unit test;this_input_node_unique_idomitted when unset (#[skip_serializing_none]) — the wire structs tolerate both absence and null.Leaf-segment consumer audit
Fixed here (the new field reaches them — all unit-test → target-model resolution):
domain/state.rsin_scope_unit_tests_with_modified,models_in_scopearm 1,unit_test_targetsdomain/scope.rs×3 (pr-diff arm: in-scope, tests-per-model, model-ids)domain/preflight.rsfirst_in_scope_test_for_modeldomain/checks.rsdetect_union_arm_coverage,tests_on_modeladapters/render.rsindex_tests_for_modelsadapters/explore.rsunit_test_counts,unit_test_paths_by_modelNot reached — follow-up needed (filed judgment: these are ref-name or model-side leaf derivations the unit-test field cannot fix):
adapters/render.rsresolve_given_ref_node(~957) — givenref()→ node by leaf name; a given input referencing a versioned model by bare name still cannot bind (and the ref parser does not handle versionedref('m', v=2)shapes). Candidate fix: per-node structuredrefslists or modelnameingestion.adapters/render.rsbuild_manifest_nodeskeys (~1088/1096) +build_model_payloadbare_name(~1751) — payload keys / DAG + section labels would display"v2"for a versioned model.adapters/explore.rsbuild_lineagenodename(~178) — explorer lineage label would display"v2".cli/mod.rsgather_model_yaml_with_reader(~525) — themodels:YAML entry name is derived by leaf split →EntryNotFoundfor versioned models.domain/check_config.rsmodel_matches(~567) — suppression rules naming a versioned model bare won't match.domain/checks.rsbranch_rollup_verdictmodel_bare(~2105) — cosmetic"v2"in verdict prose;leaf_resolves_to_seed/suggested_given_sketchare seed/ref-leaf based (seeds cannot be versioned — no impact).The root fix for the display-name family is ingesting the model node's
name(a model-side wire extension) — natural home is the #256 versions/governance slice; recommending it be folded there or filed as its own follow-up under epic #255.Gates (run directly — lefthook skips in fresh worktrees)
cargo fmt --check— passcargo clippy --all-targets --locked -- -D warnings— exit 0cargo nextest run— 1269 passedcargo test --test bdd— 163 scenarios / 1044 steps passedcargo test --test headless_toggle --locked -- --ignored— 67 passedcargo test --test headless_zero_egress --locked -- --ignored— 10 passedRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --locked— exit 0cargo deny check— okjaffle-shop,playground,diff-showcase,explore/dag.html,explore/tests.html);git diff -- examples/empty (no committed fixture uses model versions, as expected).Closes #254
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes