Repository navigation
#445 — SourceMap on the spine; derive compiled_sql + CodeMapPayload - #453
Conversation
…deMapPayload S2 of the source-map spine (epic #442). Grows the per-model source-map primitive (src/domain/source_map.rs): SpanRole (CteBody/Zone, non_exhaustive), ZoneKind, SourceMapEntry, the reserved 3-state Presence enum, and SourceMap + the compiled_slices()/node_spans() derived projections. The faithful full compiled text becomes the single source of truth; per-node compiled_sql is now a DERIVED projection of (compiled, entries) via SourceMap::from_cte_graph (the cute-dbt#40 retain-don't-recompute fold over the S1-retained per-CTE spans — no re-parse, no new compute). Adds the thin Serialize-only CodeMapPayload render projection on ModelPayload (near compiled_sql, render.rs); every compiled model now carries code_map.{compiled,node_spans} (None — key omitted — for a seed/source with no compiled code, so older fixtures stay byte-stable). KEY FIX (the no-WITH / empty-graph path): a WITH-less model emits no DAG nodes, so the assembler synthesizes ONE terminal CteBody entry over the whole text — compiled_sql now keys by the stable terminal id (was the bare model name), so node_spans.keys() == compiled_sql.keys() by construction. Blocker-1 holds: the terminal slice byte-equals the legacy post-trim terminal text (S1 advanced start.byte past the leading-trim prefix at construction). Serde note: the verbatim Zone { kind: ZoneKind } field collides with the internally-tagged enum's `kind` discriminant; renamed on the wire to `zone_kind` so Deserialize derives (ruling C6 — --context-out round-trips). TDD-first: 16 domain tests (byte-equality, no-WITH terminal entry, key-set agreement, span invariants, serde round-trip) + render tests (byte-equal slice per node, the dag-node-has-an-entry fitness check, the inlined-data + code_map omission assertions, the Zone-projection arm). source_map.rs enters the strict crap set (<=15) + the cargo-mutants targeted set: all 10 source_map mutants + both from_source_map mutants CAUGHT, 0 survivors. Goldens (consequence): the 6 compiled-model report goldens REGEN with the code_map block; the 3 explore tests.html regen too (they embed the same ModelPayload — the data-spine fact ripples into both arms). macro-heavy + diff-showcase-findings.json unchanged. Headless zero-egress (12) + toggle (123) green against the regenerated goldens. Closes #445 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
…st doc (clippy::doc_markdown) The from_source_map_projects_zone_entries_into_raw_zones test doc comment referenced SourceMap/Zone/CteBody without backticks; the pre-push clippy hook (doc_markdown, -D warnings) caught it. Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
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? |
|
Warning Review limit reached
More reviews will be available in 41 minutes and 12 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 To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. 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)
📝 WalkthroughWalkthroughIntroduces ChangesSourceMap domain primitive and render projection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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 |
![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 introduces a new per-model source-map primitive (SourceMap) in src/domain/source_map.rs as the single source of truth for raw-to-compiled code mapping, replacing the dual storage of compiled SQL and node spans. The changes include rendering projections (CodeMapPayload), integration into the model payload, and extensive unit tests. Feedback on the changes suggests using safe string slicing (.get(start..end)) instead of direct indexing in compiled_slices to prevent potential runtime panics from out-of-bounds indices or non-UTF-8 boundaries.
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 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 27934007197 -R breezy-bays-labs/cute-dbt -n report-preview-playground
open report-preview-playground/playground-report.htmlPosted by |
…act + honest docstring Council merge_after_must_fix follow-ups on S2 (SourceMap on the spine): FIX 1 (must) — wire the mandated fitness gate (AC item 6, the EdgeType-completeness-guard shape). Adds a `source-map-completeness` CI job that, over every committed example payload (the inlined `<script id="cute-dbt-data">` JSON in `examples/*-report.html` and `examples/explore*/tests.html`), asserts every `dag.nodes[].id` is a key in that model's `code_map.node_spans` (⊆), or the model has no compiled code. python3 + checkout only (no new third-party action). The unit-level `every_dag_node_has_a_code_map_entry` docstring now references the now-wired job by name instead of asserting a gate that did not exist. FIX 2 (should) — strip the transient `assertion_line:` insta header from the committed jaffle-shop chrome snapshot (non-deterministic; should not be committed). The snapshot test still passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/adapters/render.rs (1)
389-394: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUpdate the
compiled_sqldocs for parse-failure payloads.Line 390 still says parse failures leave
compiled_sqlempty, but the new unparseable-code test asserts a terminal-keyed slice and verbatimcode_map.compiled. This stale comment contradicts the S2 source-map behavior.📝 Proposed doc update
- /// Per-node compiled SQL, keyed by node id (CTE name or the stable - /// terminal id for the final select). Empty when the CTE engine could - /// not parse (the model card still renders the metadata + tests + an - /// empty DAG). A DERIVED PROJECTION of [`Self::code_map`]'s source map + /// Per-node compiled SQL, keyed by node id (CTE name or the stable + /// terminal id for the final select). For compiled models, this is a + /// DERIVED PROJECTION of [`Self::code_map`]'s source map; parse failures + /// can still surface a terminal-keyed fallback slice while the DAG is empty.🤖 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 389 - 394, The documentation comment for the compiled_sql field incorrectly states that it is empty when the CTE engine fails to parse, but the new unparseable-code test demonstrates that compiled_sql actually contains a terminal-keyed slice of verbatim code_map.compiled data even in parse-failure scenarios. Update the doc comment for the compiled_sql field to accurately reflect this behavior, removing the statement about being empty on parse failures and clarifying that it contains a terminal-keyed slice of code_map.compiled consistent with the S2 source-map behavior, as verified by the existing test.
🤖 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 @.github/workflows/ci.yml:
- Around line 311-336: Add a validation check to fail the job when zero DAG-node
checks are executed. After the existing checks for empty files and violations,
add a condition that checks if node_checks equals zero and prints an error
message using the same pattern as the other error handlers (print with ::error::
prefix and sys.exit(1)). This ensures that if all models lack code_map due to a
regression or glob drift, the job fails rather than silently reporting success
with the misleading node_checks value of zero in the final message.
In `@src/adapters/render.rs`:
- Around line 3914-3915: The `None if !full_compiled_code.is_empty()` match arm
in the `build_compiled_sql` function is unreachable dead code that should be
removed since `SourceMap::from_cte_graph` returns `None` only when `compiled` is
empty, making the guard condition always false in that case. Remove this match
arm from the `build_compiled_sql` function definition, then remove the unused
`model_name` parameter from the function signature. Finally, update the call
site at line 3914 to remove the `&bare_name` argument from the
`build_compiled_sql` invocation since that parameter is no longer needed.
In `@src/domain/source_map.rs`:
- Around line 156-180: The condition checking `entries.is_empty()` is incorrect
because it will also match graphs where nodes exist but all their source_span()
values are None, causing the code to incorrectly synthesize a terminal span
mapping when it should preserve the absence of spans. Replace the
entries.is_empty() check with a check on the graph itself
(graph.nodes().is_empty()) to ensure terminal span synthesis only happens when
the graph truly has no nodes, not when nodes exist but lack span information.
---
Nitpick comments:
In `@src/adapters/render.rs`:
- Around line 389-394: The documentation comment for the compiled_sql field
incorrectly states that it is empty when the CTE engine fails to parse, but the
new unparseable-code test demonstrates that compiled_sql actually contains a
terminal-keyed slice of verbatim code_map.compiled data even in parse-failure
scenarios. Update the doc comment for the compiled_sql field to accurately
reflect this behavior, removing the statement about being empty on parse
failures and clarifying that it contains a terminal-keyed slice of
code_map.compiled consistent with the S2 source-map behavior, as verified by the
existing test.
🪄 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: 8c905ac3-3a8e-4ca7-b1f2-f2b2f4731bad
⛔ Files ignored due to path filters (1)
tests/snapshots/render_integration__rendered_chrome_jaffle_shop.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
.cargo/mutants.toml.github/workflows/ci.ymlcrap4rs.tomlexamples/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/render.rssrc/domain/mod.rssrc/domain/source_map.rs
… dead compiled_sql fallback, honest terminal-synthesis on graph emptiness Three independently-verified CodeRabbit review fixes on the S2 source-map spine: - ci.yml source-map-completeness gate now FAILS CLOSED: track models_with_code_map and exit(1) if models_with_code_map == 0 or node_checks == 0. A regression dropping code_map from every compiled model would otherwise report a silent false pass. Committed examples execute 509 checks over 10 payloads (106 code_map-bearing models), so zero checks is a real regression signal. The glob-drift tripwire stays. - render.rs build_compiled_sql: remove the dead `None if !full_compiled_code.is_empty()` arm. SourceMap::from_cte_graph returns None ONLY when the compiled string is empty, so that guard is never reachable (the same compiled_code feeds both). Drops the now-unused model_name + full_compiled_code params; clippy --all-targets --locked stays green. - source_map.rs from_cte_graph: gate terminal-span synthesis on graph.nodes().is_empty() instead of entries.is_empty(). entries is also empty when the graph HAS nodes but every source_span() is None, where the old code FABRICATED a whole-text terminal span (a false claim); preserve honest absence instead. Adds a regression test. Zero golden movement (every parsed node in the committed examples retains a span). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199AmBCec5kyVEEF1Qd7TSF
|
✅ Merged S2 (#445) — the SourceMap on the spine. What landed: the per-model Quality: council review (cao/ceng/cqo clean, cpo concerns) → Follow-ups filed: #454 (fallible Merged autonomously under the overnight build mandate. Next: S3+CLL-1 (#446) — bidirectional sync + column context — and CLL-2 (#447), both building on this. |
1 participant
Summary
S2 of the source-map spine (epic #442). Grows the per-model source-map primitive and makes the faithful full compiled text the single source of truth that
compiled_sqlnow derives from.src/domain/source_map.rs—SpanRole(CteBody/Zone,#[non_exhaustive]),ZoneKind,SourceMapEntry(is_compiled_in), the reserved 3-statePresenceenum, andSourceMapwith thecompiled_slices()/node_spans()derived projections, plus the pure-domainfrom_cte_graphassembler (the cute-dbt#40 retain-don't-recompute fold over the S1-retained per-CTE spans — no re-parse).ModelPayload.compiled_sqlis now a DERIVED projection of the source map; new Serialize-onlyCodeMapPayload(compiled+node_spans+ reservedraw_zones) onModelPayload, present for every compiled model (None— key omitted — for a seed/source with no compiled code, so older fixtures stay byte-stable).Key fix — the no-WITH / empty-graph path
A WITH-less model emits no DAG nodes, so the assembler synthesizes ONE terminal
CteBodyentry over the whole text.compiled_sqlnow keys by the stable terminal id (was the bare model name), sonode_spans.keys() == compiled_sql.keys()by construction. Blocker-1 holds: the terminal slice byte-equals the legacy post-trim terminal text (S1 advancedstart.bytepast the leading-trim prefix at construction).Serde note
The verbatim
Zone { kind: ZoneKind }field collides with the internally-tagged enum'skinddiscriminant; the field is renamed on the wire tozone_kindsoDeserializederives (ruling C6 —--context-outround-trips, the Claude Design harness re-ingests).Gates run (all green, judged by raw exit code)
cargo fmt --all -- --check,cargo clippy --all-targets --locked -- -D warnings,RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --locked,cargo deny check— clean.cargo nextest run— 2412 passed, 135 skipped.cargo test --test bdd— 242 scenarios / 1645 steps passed.file://Chromium):headless_zero_egress12 passed (zero external requests against the regenerated goldens);headless_toggle123 passed.crap4rsstrict —source_map.rsadded to the strict (<=15) set; max CRAP 3.14, exit 0.cargo-mutants—source_map.rsadded to the targeted examine set; all source_map.rs mutants CAUGHT + bothfrom_source_mapmutants CAUGHT, 0 survivors in the slice. (One pre-existing unrelated survivorrender.rs:1196 ManifestColumnPayloadis outside this diff.)TDD-first
16 domain tests (compiled byte-equality, no-WITH terminal entry, key-set agreement, span invariants, serde round-trip, Zone round-trip despite the inner
kindfield) written before the impl, plus render tests: byte-equal slice per node, the dag-node-has-a-CteBody-entry fitness check, the inlined-data assertion +code_mapomission for uncompiled nodes, and the Zone-projection arm.Golden impact (consequence, not driver)
The 6 compiled-model report goldens REGEN with the additive inlined
code_mapblock (jaffle-shop, playground, diff-showcase, seed-showcase, prdiff-minidag, comments-showcase).macro-heavy+diff-showcase-findings.jsonare unchanged. The 3 exploretests.htmlgoldens regen too — they embed the sameModelPayload, so the data-spine fact ripples into both arms (regen + byte-verify BOTH). Therender_integrationinsta snapshot is updated; its diff is exactly the additivecode_map.{compiled,node_spans}block (byte-equality verifiable from the data: e.g.node_spans["(final select)"]byte 227..248 = the post-trim terminal slice). No paireddbt-project/construct change — S2 adds no new dbt-construct rendering surface (it is the data spine that S3 consumes); the existing showcase fixtures already exercise thecode_map.Closes #445
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests