Repository navigation
#245 — accept double-quoted ref()/source() given inputs in Rust parsers - #248
Conversation
|
Warning Review limit reached
More reviews will be available in 33 minutes and 33 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. 📝 WalkthroughWalkthroughThe PR extends dbt-style given input parsers to accept both single-quoted and double-quoted ChangesQuote support in dbt-style ref() and source() given inputs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 4 individual chapters for you:
Chapters generated by Stage for commit 65bee12 on Jun 12, 2026 12:39am UTC. |
![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 adds support for both single and double quotes in dbt unit test ref and source inputs, resolving issue cute-dbt#245. It introduces a helper function strip_matching_quotes to validate and strip matching quote pairs, and updates the parsing logic in parse_ref_name and parse_source_ref. Additionally, new unit and integration tests have been added to ensure correct behavior. There are no review comments, so I have no feedback to provide.
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.
📄 Rendered report previewAll golden examples regenerated cleanly. 🟡 Golden examplesCommitted to
🐶 Live dogfood previewThis PR's own
▶ 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 27386768645 -R breezy-bays-labs/cute-dbt -n report-preview-playground
open report-preview-playground/playground-report.htmlPosted by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/adapters/render.rs`:
- Around line 197-199: The parsing of source(...) currently uses
inside.split_once(',') which accepts inputs with extra commas (e.g.,
source('a','b','c')); before calling split_once ensure there is exactly one
comma by checking the comma count (e.g., inside.matches(',').count() == 1) and
return an error if not, then call inside.split_once(',') and proceed to call
strip_matching_quotes on both trimmed parts; update the error path to clearly
reject malformed argument lists so strip_matching_quotes is only invoked when
there are exactly two arguments.
🪄 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: b1a133ee-0403-4e42-b8c1-8fc7c53b46ea
📒 Files selected for processing (2)
dbt-project/models/marts/_marts__models.ymlsrc/adapters/render.rs
CodeRabbit (PR #248): parse_source_ref used split_once(','), so a malformed 3-arg call like source('a','b','c') split to first='a', second='b','c' — and strip_matching_quotes only checks the end characters, so the second fragment stripped to the garbage pair ("a", "b','c") instead of None. Probe-verified RED before fixing (left: Some(("a", "b','c"))). splitn(3, ',') + reject when a third part exists. Pinned for both quote styles plus the trailing-comma shape; the existing comma-inside-quoted-name pins are kept (that path now rejects via the part count or, for two-part splits, the matching-quote strip — fail-open either way). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dbt accepts both quote styles in a unit-test given's input: and dbt-fusion ships the authored string verbatim on the manifest wire (verified 2026-06-11 against a real fusion 2.0.0-preview.177 compile — see issue #245). The Rust parsers (parse_ref_name / parse_source_ref) stripped single quotes only, so a double-quoted given lost its bound_to_node binding and its column_meta resolution. - new strip_matching_quotes helper: one pair of MATCHING quotes ('…' or "…"); mixed pairs and unbalanced quotes stay None (fail-open), applied per argument for source(a, b) - parser test family extended: double-quote accepts, mixed-quote None pins, per-arg mixed-style source args, double-quoted empty / unmatched variants; the stale returns_none_on_double_quoted pins (which pinned the bug) rewritten to assert the accepting behavior - AC4 render-integration test: render_report binds a double-quoted given (bound_to_node in the payload) and populates its input model's column_meta - dogfood: one given in dbt-project flipped to ref("stg_payments") so the live prdiff-preview proves the binding - comma-split fail-open comment updated for both quote styles Closes #245 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CodeRabbit (PR #248): parse_source_ref used split_once(','), so a malformed 3-arg call like source('a','b','c') split to first='a', second='b','c' — and strip_matching_quotes only checks the end characters, so the second fragment stripped to the garbage pair ("a", "b','c") instead of None. Probe-verified RED before fixing (left: Some(("a", "b','c"))). splitn(3, ',') + reject when a third part exists. Pinned for both quote styles plus the trailing-comma shape; the existing comma-inside-quoted-name pins are kept (that path now rejects via the part count or, for two-part splits, the matching-quote strip — fail-open either way). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f53365d to
5333bec
Compare
…t arity PR #250 (#247 model YAML drawer) widened render_report with the model_yaml map (arg 9); the rebase kept the AC4 in-process render test at the old 13-arg call. Pass the same neutral &HashMap::new() the other in-tree test call sites pass — assertions unchanged (the test still proves the double-quoted given's bound_to_node + column_meta binding). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion (#272) given_input_leaf scanned for single-quoted arguments only (rfind('\'')), so a double-quoted given input — ref("x"), which dbt accepts and both engines ship verbatim on the manifest wire (cute-dbt#245) — lost its leaf binding entirely. In the union.arm-coverage check a fed arm then read as provably unfed and degraded to a FALSE UNCOVERED nag (worse than the issue's stated Unknown); the join checks' side_rows consumer mocked the side empty the same way. - the right-to-left scan now accepts either quote style under the matching-quote rule (open/close must be the SAME character; mixed or unbalanced pairs stay None, fail-open) — the domain twin of the renderer's strip_matching_quotes contract from PR #248, kept local because domain never imports adapters and the scan deliberately tolerates trailing non-string kwargs (ref('orders', v=2)) - TDD: leaf-extraction test family (single-quote pins stay, double- quote accepts, per-argument mixed styles for source(), mixed/ unbalanced-quote None pins) + a detector-level twin pinning that a double-quoted given classifies arm-coverage identically to its single-quoted twin (RED showed Uncovered vs Covered) - dogfood (the paired-fixture rule): both playground mart_dq_summary tests' stg_synthea__medications givens flipped to ref("…") in current+baseline manifests AND the paired source YAML (same-revision contract; 1:1 in-place line edits keep the pr-diff showcase hunk numbers unshifted), so the committed goldens prove union[combined_metrics] stays COVERED through a double-quoted given - goldens regenerated: playground / diff-showcase / explore tests.html carry the quote flip (bound_to_node intact in the payload); jaffle-shop + explore dag.html byte-identical - BDD steps' input capture widened to greedy (.+) so feature lines can carry a double-quoted input string; MANIFEST.toml SHAs + provenance notes updated Closes #249 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
The Rust given-input parsers in
src/adapters/render.rs(parse_ref_name,parse_source_ref) stripped single quotes only. dbt accepts both quote styles in a unit-test given'sinput:and dbt-fusion ships the authored string verbatim on the manifest wire (verified 2026-06-11 against a real fusion 2.0.0-preview.177 compile of the dogfood project — evidence in issue #245; no re-verification needed). A double-quoted given therefore lost itsbound_to_nodebinding and itscolumn_metaresolution.Fix: a new
strip_matching_quoteshelper strips one pair of matching string-literal quotes ('…'or"…"). Open/close must be the same character; mixed pairs (ref("x')) and unbalanced quotes stayNone— fail-open. Forsource(a, b)the rule applies per argument (each arg is its own string literal, so mixed-style args likesource("a", 'b')parse). Single-quote behavior is byte-for-byte unchanged. This is the Rust twin of the JSgivenOwnerLabelfix that landed in PR #244 (same matching-quote backreference rule).Acceptance criteria — evidence
parse_ref_nameacceptsref("x"); mixed quotes stayNone—parse_ref_name_accepts_double_quoted_name,parse_ref_name_returns_none_on_mixed_quotes, plus double-quote variants added to the unmatched/empty pins. All pre-existing single-quote pins kept unchanged.parse_source_refacceptssource("a", "b")and mixed-style args per-arg —parse_source_ref_accepts_double_quoted_names,parse_source_ref_accepts_mixed_style_args,parse_source_ref_returns_none_on_mixed_quotes_within_an_arg.*_returns_none_on_double_quoted_*pins (which pinned the bug) rewritten to assert the accepting behavior, citing the adapters: parse_ref_name / parse_source_ref reject double-quoted ref()/source() given inputs (fusion ships authored quotes verbatim) #245 fusion wire evidence.render_report_binds_double_quoted_given_and_populates_column_meta: builds domain objects with a double-quotedgiven.input, callsrender_report, asserts the rendered HTML carries"bound_to_node":"stg_payments_src"and the input model's column-description marker. (Implemented in-process per the concurrent-builder file-ownership split;tests/headless_toggle.rsuntouched. Headless coverage is not irreplaceable here — the binding markers are payload facts the in-process render asserts end-to-end, and the live prdiff-preview exercises the real browser path.)Discovery answers
parse_source_ref(the issue attributes it tofind_source_import_node_id, but that function has no comma-split; the comment sits above thesplit_once(',')in the parser). The property still holds with both quote styles: a comma inside a quoted name (either style) leaves the first split fragment with an unbalanced or mismatched quote pair, which the matching-quote strip rejects — fail-open by construction. The comment was updated to say so, and the property is now pinned for the double-quoted case too (source("a,b", "c")→Noneinparse_source_ref_returns_none_on_unmatched_quotes).src/domain/checks.rsgiven_input_leaf(~L977) extracts "the last single-quoted argument" viarfind('\'')(the renderer-binding twin, duplicated in domain because domain never imports adapters). For a double-quoted given it returnsNone, so union arm-coverage classification degrades toArmCoverage::Unknown— fail-open (never a wrong finding), but the C3 arm-coverage check loses precision for double-quoted givens. Out of scope for this tight diff; recommend a small follow-up issue mirroringstrip_matching_quotesthere. Not gaps:parse_yaml_scalar(unit_test_yaml.rs) already handles both quote styles;literal_cell(unit_test_table.rs) is correct SQL-literal semantics (single-quoted = string, double-quoted = identifier) and must stay single-quote-only; the remainingref('…')hits inchecks.rsare recommendation-sketch output formatting, not input parsing. The JS twingivenOwnerLabelwas already fixed in PR #240 — review-2 render defects: tooltip overflow, given-header tooltips, covered-by contrast, overrides-badge clip, baseline banner #244.Dogfood
Exactly one given in
dbt-project/models/marts/_marts__models.ymlflipped fromref('stg_payments')toref("stg_payments")(the same flip used for the fusion wire verification in #245), with acute-dbt#245 dogfoodcomment. The live prdiff-preview on this PR self-renders from an ephemeral CI-compiled fusion manifest and proves the binding (bound_to_node+column_meta) on the real wire. Nothing under anytarget/directory and no root_path-bearing artifact is committed.Goldens
All four golden examples (
jaffle-shop,playground,diff-showcase,explore) regenerated locally exactly as the CIexample-report-checkmatrix does;git diff --text -U0 -- examples/is empty (no committed project uses double quotes, as expected).Gates (run directly — lefthook skips in fresh worktrees)
cargo fmt --check— passcargo clippy --all-targets --locked -- -D warnings— exit 0cargo nextest run— 1232 passedcargo test --test bdd— 160 scenarios / 1032 steps passedcargo test --test headless_toggle --locked -- --ignored— 63 passedcargo test --test headless_zero_egress --locked -- --ignored— 10 passedRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --locked— exit 0cargo deny check— exit 0Merge order: after PR for #247 (concurrent render.rs work); expect a rebase before merge.
Closes #245
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests