Repository navigation
#448 — raw-Jinja zone scanner + SpanRole::Zone facts + 3-state presence (CORE) - #465
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRedesigns the ChangesRaw Jinja Zones and Column Source Map
Sequence Diagram(s)sequenceDiagram
participant build_model_payload
participant locate_raw_zones
participant resolve_zone_compiled
participant SourceMap
participant CodeMapPayload
build_model_payload->>locate_raw_zones: raw_code
locate_raw_zones-->>build_model_payload: Vec<ZoneFact>
build_model_payload->>SourceMap: from_cte_graph(graph)
build_model_payload->>SourceMap: append_zone_entries(raw_code, compiled_sql)
note over SourceMap: resolve_zone_compiled per zone<br/>binds compiled SourceSpan via longest anchor
build_model_payload->>CodeMapPayload: from_source_map(sm)
note over CodeMapPayload: gather_raw_zones → RawZonePayload[]<br/>presence via SourceMapEntry::presence()<br/>node_id via smallest-span containment
CodeMapPayload-->>build_model_payload: { node_spans, column_spans, raw_zones }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 |
![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 intra-model column lineage (CLL-2) and raw-Jinja control zones (Z1+Z2). It adds domain models for column edges, spans, and scopes, and implements a projection-provenance pass in the CTE engine alongside a hand-rolled Jinja tag-boundary scanner in the adapter layer. Feedback on the changes highlights two instances in src/adapters/render.rs where unstable Rust let-chains (&& let) are used, which will cause compilation failures on stable toolchains; refactoring these to nested if let statements is recommended.
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: 4
🧹 Nitpick comments (1)
src/adapters/render.rs (1)
7676-7886: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd regression tests for the scanner/resolver edge cases above.
The new coverage is strong for the main paths, but please pin the false-positive
some_is_incremental_flag, orphanelse/elif,%}inside quoted tag text, and duplicated/outside-zone anchor cases so the fail-closed and never-a-false-claim guarantees stay locked.🤖 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 7676 - 7886, Add three new regression test functions to cover the edge cases mentioned: a test for the false-positive some_is_incremental_flag case (e.g., a string containing "is_incremental" that is not a tag), a test for orphan else/elif statements without matching if tags, and a test for duplicated or outside-zone anchor cases where an anchor string appears multiple times or outside the zone boundary. Each test should verify that the scanner and resolver functions (locate_raw_zones, append_zone_entries, and resolve_zone_compiled) maintain their fail-closed behavior and never-a-false-claim guarantees by returning empty results or None when encountering these invalid patterns.
🤖 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 3493-3494: In the match statement handling tag roles, the
TagRole::Mid variant is currently ignored without validation, allowing orphan
mid-block tags like else or elif to pass through unchecked. Modify the handler
for TagRole::Mid to validate that there is a corresponding opener on the current
stack before allowing the tag to be processed. If no valid opener exists on the
stack for the mid-tag, return an empty vector to fail closed. Keep
TagRole::Other as-is but separate it from the Mid validation logic so it
continues to be silently consumed without affecting pairing.
- Around line 465-475: The resolve_zone_compiled function currently returns the
first occurrence of the longest fragment from raw_span found in compiled, which
can be ambiguous if that fragment appears elsewhere. After calling find on the
anchor, add a check to verify that this anchor does not also appear at other
locations in compiled that would be outside the expected zone boundaries. If the
anchor is found at multiple locations or appears outside the context of
raw_span, return None instead of a potentially incorrect span. Consider
validating that the discovered position in compiled corresponds specifically to
the zone being resolved, not to a stray occurrence of the same literal
elsewhere.
- Around line 3618-3626: The current implementation in the "if" case uses
inner.contains("is_incremental") which incorrectly matches substring occurrences
like plain variable names (e.g., some_is_incremental_flag) or quoted text, when
it should only match actual function calls to is_incremental(). Replace the
simple substring check with a more precise pattern matcher that looks for the
identifier is_incremental followed by optional whitespace and an opening
parenthesis, ensuring it respects identifier boundaries and ignores quoted
content so that only genuine is_incremental() function calls are recognized as
BlockKind::IncrementalGuard.
- Around line 501-519: The current implementation of the Jinja construct closer
in the section starting with "let mut j = i + 2" uses a naive literal scan that
doesn't account for quoted strings. This causes constructs like `{% if x == '%}'
%}` to terminate prematurely at the first `%}` found inside the string literal
instead of at the actual closing delimiter. Replace the simple while loop that
scans for the close_delim followed by b'}' with the existing quote-aware scanner
helper functions to properly skip over quoted strings while searching for the
actual closing boundary of the Jinja construct.
---
Nitpick comments:
In `@src/adapters/render.rs`:
- Around line 7676-7886: Add three new regression test functions to cover the
edge cases mentioned: a test for the false-positive some_is_incremental_flag
case (e.g., a string containing "is_incremental" that is not a tag), a test for
orphan else/elif statements without matching if tags, and a test for duplicated
or outside-zone anchor cases where an anchor string appears multiple times or
outside the zone boundary. Each test should verify that the scanner and resolver
functions (locate_raw_zones, append_zone_entries, and resolve_zone_compiled)
maintain their fail-closed behavior and never-a-false-claim guarantees by
returning empty results or None when encountering these invalid patterns.
🪄 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: 6e06a90c-fba9-4635-8726-ed69b76907be
⛔ Files ignored due to path filters (1)
tests/snapshots/render_integration__rendered_chrome_jaffle_shop.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
examples/comments-showcase-report.htmlexamples/diff-showcase-report.htmlexamples/explore-macro/tests.htmlexamples/explore-seed/tests.htmlexamples/explore/tests.htmlexamples/jaffle-shop-report.htmlexamples/playground-report.htmlexamples/prdiff-minidag-report.htmlexamples/seed-showcase-report.htmlsrc/adapters/cte_engine.rssrc/adapters/render.rssrc/domain/cte.rssrc/domain/mod.rssrc/domain/source_map.rs
…+ 3-state presence (CORE)
Hand-rolled tag-boundary scanner over raw_code in the adapter (locate_raw_zones,
beside macro_call_sites), mirroring dbt-fusion's JinjaLayoutEvent model with ZERO
new dependency. Pairs {% if is_incremental() %} / {% for %} start/mid/end via a
depth-stack (not first-endif-wins), handling {%- -%} whitespace control,
string-literal-aware delimiter skipping, {#…#} comments, and nesting; fail-closed
(malformed/unbalanced → empty vec, never panic). A plain {% if %} pairs (so it
never unbalances a sibling zone) but emits no v0.1 zone (L9).
Each ZoneFact resolves in build_model_payload into a SpanRole::Zone SourceMapEntry
(compiled span by honest token-location anchored on the longest literal SQL
fragment; absent ⇒ None = the honest "pruned this build" verdict). The 3-state
presence() + Structural detection lands CORE in src/domain/source_map.rs
(un-gated, NO Experiment::RawZoneSync). RawZonePayload (§3.1) is projected through
CodeMapPayload.raw_zones in a single gather_raw_zones fn: the presence verdict is
derived ONCE in Rust and emitted as a string (compiled_in/compiled_out/structural)
the prototype renders verbatim, and the owning node back-ref is derived via
contains_range — null when compiled_out (type-incapable of an edge,
never-a-false-claim).
Goldens: the playground manifest's fct_encounters_incremental model carries a real
{% if is_incremental() %} guard whose WHERE predicate compiled IN nested inside the
terminal select body → an honest `structural` zone bound to (final select). The
playground / diff-showcase report goldens + the explore / explore-macro tests.html
goldens each gain exactly one raw_zones entry; regenerated + byte-verified both arms.
TDD-first: Z1 scanner (incremental guard, for-loop, nested-by-depth-stack,
whitespace-trim, comment-swallowed inner tag, string-literal %} skip, unbalanced →
empty, plain-if pairs-no-zone) + Z2 (incremental compiled-OUT → None/compiled_out/
node_id null, Shape-A for-loop Structural, the compiled==None ⇒ no-edge fitness
test over an exhaustive cte_spans cube) + presence()/to_wire unit tests.
Gates: fmt, clippy --locked --all-targets, nextest (2456), bdd (242 scenarios),
cargo doc -D warnings, cargo deny, headless zero-egress (12) + toggle (125) all green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
9fc73c2 to
c71b730
Compare
📄 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 27944128729 -R breezy-bays-labs/cute-dbt -n report-preview-playground
open report-preview-playground/playground-report.htmlPosted by |
…il-closed orphan tags + is_incremental call-match + nested if-let)
Four verified review fixes to the raw-Jinja zone scanner in
src/adapters/render.rs (locate_raw_zones + the zone resolution path),
each with a covering test:
FIX A (gemini HIGH x2, stability + perf): the two let-chain `&& let`
sites (locate_raw_zones zone-emit + build_model_payload zone-append)
become DELIBERATELY nested `if let` (not let-chains). Skips the
prefix-scanning byte_span when zone_kind() is None (perf win for plain
`{% if %}`), and sidesteps the unstable let_chains question on MSRV 1.88.
collapsible_if suppressed at those two sites only, with rationale.
FIX B (CodeRabbit Major, never-a-false-claim): resolve_zone_compiled is
now AMBIGUITY-SAFE. It walks the literal-fragment candidates (longest
first) and binds only a fragment that occurs EXACTLY ONCE in `compiled`;
a multiply-occurring anchor (a pruned guard whose literal also lives
outside the zone region) degrades to None (honest CompiledOut) instead
of fabricating a false Some/CompiledIn at the first occurrence. New
literal_fragments() replaces the head-only longest_literal_fragment.
FIX C (CodeRabbit Major, fail-closed): an orphan mid-block tag
(`{% else %}` / `{% elif %}` with no opener on the depth-stack) now
returns an EMPTY vec — malformed Jinja fails closed, never letting a
later zone emit from a broken stream.
FIX D (CodeRabbit Major, classifier correctness): the IncrementalGuard
classification matches an ACTUAL `is_incremental()` CALL via a new
identifier-boundary, quote-aware mentions_is_incremental_call(), not a
bare substring. `some_is_incremental_flag` and a quoted
`'is_incremental'` no longer misclassify; `is_incremental()` (with
optional whitespace before `(`) still does.
No golden moved: the playground fct_encounters_incremental zone (a real
is_incremental() call with an unambiguous anchor) stays `structural`;
playground + diff-showcase + findings regenerate byte-identically. One
existing test fixture (append_zone_entries_shape_a_for_loop_is_structural)
was given a unique loop-body anchor so it stays bindable under FIX B (a
loop whose body literal repeats verbatim per iteration is genuinely
unbindable — covered by the new ambiguity test).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
…pass crap4rs (CC 40 -> <=15)
The just-added identifier-boundary, quote-aware is_incremental() call
matcher was written as one branchy char-scanner: cyclomatic complexity
40 -> CRAP 40.73, over the crap4rs gate (default 25; render.rs strict
15). Because CRAP = CC^2(1-cov)^3 + CC >= CC, coverage cannot save it —
the cyclomatic complexity itself had to drop.
Behavior-preserving decomposition into small single-purpose private
helpers, each independently low-CC:
- is_word_byte — the identifier-char predicate.
- token_left_boundary_ok / token_right_boundary_ok — the two identifier
boundary checks at the token edges.
- call_paren_follows — skip optional whitespace, require `(`.
- offset_in_string_literal + step_string_scan — the quote-aware
in-literal test (single/double quotes, `\`-escapes).
mentions_is_incremental_call now iterates match_indices("is_incremental")
and returns true on the first occurrence passing all four predicates.
Result (crap4rs --config crap4rs.toml --coverage lcov.info => PASS,
0 above threshold): mentions_is_incremental_call CC 2 / CRAP 2.0;
every helper CC <= 10, all under the strict 15. Per-helper unit tests
added so step_string_scan reaches 100% coverage (CC 10 needs it to clear
the keyed gate). The FIX D test classify_is_incremental_matches_an_actual_call_not_a_substring
stays green; goldens unchanged (pure refactor).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
|
✅ Merged the raw-Jinja zone scanner (#448) — the source-map spine now maps control-flow zones. What landed: a hand-rolled, This was the most heavily reviewed slice of the run. The multi-layer review caught: a branch-stacking conflict (stale local main), 4 scanner edge cases (an anchor ambiguity that could fabricate a "compiled-in" claim, an orphan-tag fail-closed gap, a substring-vs-call classifier bug, an unstable-feature/perf refactor), and a crap4rs complexity failure (CC 40 → 2). All fixed before merge. Next: #464 (Z3 — a bolero fuzz target on the scanner + a dogfood incremental/for-loop model to make zones visible in a committed golden). Merged autonomously under the overnight build mandate. |
1 participant
Closes #448
Summary
Raw-Jinja zones land as CORE (Z1+Z2 from the shaping §7), riding the merged source-map spine (S1 #444 / S2 #445 / S3 #446 / CLL-2 #447 on main).
locate_raw_zones(raw_code) -> Vec<ZoneFact>is a hand-rolled tag-boundary scanner insrc/adapters/render.rsbesidemacro_call_sites, mirroring dbt-fusion'sJinjaLayoutEventmodel with zero new dependency (cargo-deny / zero-egress / domain-purity all untouched). A depth-stack pairs{% if … %}/{% for … %}start/mid/end (not first-endif-wins), handling{%- -%}whitespace control, string-literal-aware%}skipping,{#…#}comments, and nesting. Fail-closed: malformed/unbalanced → empty vec, never a panic/hang. A plain{% if %}pairs (so it never unbalances a sibling zone) but emits no v0.1 zone (L9 — only the incremental guard + for-loop are zones).ZoneFactresolves inbuild_model_payloadinto aSpanRole::ZoneSourceMapEntry: thecompiledspan by honest token-location (the longest literal SQL fragment found incompiled_code; absent ⇒None= the honest "pruned this build" verdict, never fabricated).presence()+Structuraldetection un-gated CORE insrc/domain/source_map.rs(NOExperiment::RawZoneSync).RawZonePayload(§3.1) is projected throughCodeMapPayload.raw_zonesin a singlegather_raw_zonesfn: the 3-state verdict is derived once in Rust and emitted as a string (compiled_in/compiled_out/structural) the prototype renders verbatim; the owningnode_idback-ref is derived viacontains_range—nullwhencompiled_out(type-incapable of an edge, never-a-false-claim).Golden impact
The playground manifest's
fct_encounters_incrementalmodel carries a real{% if is_incremental() %}guard whoseWHEREpredicate compiled IN, nested inside the terminal select body → an honeststructuralzone bound to(final select). Four goldens gain exactly oneraw_zonesentry and were regenerated + byte-verified against deterministic renderer output:examples/playground-report.htmlexamples/diff-showcase-report.htmlexamples/explore/tests.htmlexamples/explore-macro/tests.htmlThe
dag.htmlpages anddiff-showcase-findings.jsonstay byte-identical. No fixtures added (synthetic-fixture gate untouched).TDD-first tests
%}skip, unbalanced → empty, plain-if pairs-no-zone.None/compiled_out/node_idnull; Shape-A for-loop →Structural; thecompiled == None ⇒ no node edgefitness test over an exhaustivecte_spanscube;presence()3-state +to_wireunit tests.Gates (all green)
cargo fmt --check·cargo clippy --all-targets --locked -D warnings·cargo nextest run(2456) ·cargo test --test bdd(242 scenarios) ·RUSTDOCFLAGS=-D warnings cargo doc --no-deps --locked·cargo deny check· headlesszero_egress(12, --ignored on real Chrome) +toggle(125).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements