#40 — feat(domain+adapters): engine pre-computes CTE-body shape facts as POD (Option C)

[cmbays]

Summary

Replaces the renderer's whitespace-tokenizing is_simple_from_select
and extract_table_leaf_refs heuristics with engine-computed POD
facts on CteNode (Option C from the /shape pass).

The CTE engine already parses each model's compiled SQL into a
sqlparser::ast::Query to build the edge graph; this PR adds a small
AST walk over each CTE body during that same single-parse pass to
populate two new domain fields:

• CteNode::is_simple_from_shape: bool — true when the body is a
  single SELECT FROM <Table> with no joins (the import-CTE shape).
• CteNode::body_leaf_table_refs: Vec<String> — lowercased leaf table
  identifiers from any FROM/JOIN in the body.

Both fields are #[serde(default)] for backward compatibility. The
renderer reads them via accessors and never holds a parser, never
sees an AST, and never re-parses the slice. Domain stays POD-only by
construction; the hexagonal layering invariant is strengthened, not
violated.

Empirical AC correction (issue body amended in place)

Issue #40 originally cited select 'from' as col from x as a
heuristic false positive. The /shape pass empirically traced this
against the current heuristic (/tmp/heuristic_check.rs in the
worktree, captured in shape-pr-12.5-engine-api.md) and found that
both the heuristic and the AST classify it as simple — correctly, it
is a single-source SELECT. The actual false positive is comma
cross-join:

select * from x, y      -- heuristic says "simple" (1 from keyword)
                        -- AST says "not simple" (2 source relations)

Issue #40's body has been amended (gh issue edit 40, 2026-05-24) to
reflect this. The Gemini-2 review on PR #38 was directionally correct
("use the AST") but cited the wrong demonstration — preserved in
memory feedback_bot-review-suggestions-must-be-verified.

Net change

File	Lines
src/adapters/render.rs	+9 / -481
src/adapters/cte_engine.rs	+201 (AST walk + 6 new tests)
src/domain/cte.rs	+82 (POD fields + builder + accessors + tests)
ARCHITECTURE.md	+15 (POD-pattern paragraph in §1)
Total	+337 / -451 (net -114 lines)

Net code REMOVAL. The renderer loses is_simple_from_select,
extract_table_leaf_refs, is_from_or_join_keyword,
extract_leaf_table_identifier, is_identifier_char,
strip_sql_comments, and helpers copy_string_literal /
skip_line_comment / skip_block_comment. ~30 unit tests on those
functions are also removed.

Measurements

Gate	Result
cargo nextest run --all-targets --locked	293/293 pass
cargo test --test bdd	27 scenarios / 153 steps pass
cargo clippy --all-targets --locked -- -D warnings	clean
cargo fmt --all --check	clean
cargo deny check	ok
cargo doc --document-private-items --locked (RUSTDOCFLAGS=-D warnings)	clean
crap4rs --coverage lcov.info	PASS, worst 8.0 (was 11.0 pre-PR)
Snapshot tests on jaffle-shop fixtures	unchanged (AST agrees with heuristic on every fixture CTE)

Both src/adapters/render.rs and src/adapters/cte_engine.rs stay
comfortably under their crap4rs strict-15 thresholds.

Decision record

ADR amendment added to
ops/decisions/cute-dbt/adr-mvp-architecture.md under ADR-1
("Single-crate hexagonal layout with inward-dependency discipline").
The amendment records the build-phase precedent:

AST-derived structural facts flow through the domain as POD, not
through trait, port, or AST reference. The single parser pass in
the adapter is the single source of truth; the renderer reads the
POD facts and contributes zero re-parse cost. New facts of this
kind are additive POD fields with #[serde(default)]; no domain
layer ever pulls in sqlparser.

ARCHITECTURE.md §1 carries the public-repo paragraph linking back
to the rule.

Options considered (rejected in /shape)

• A — renderer re-parses each CTE body slice. Pushes parser
  awareness into the renderer (already on crap4rs strict-15); slice
  is name AS (...) form, requires string surgery before
  Parser::parse_query accepts it.
• B — CteNode exposes parsed AST. Explicit hex-layering
  violation (domain depends on sqlparser); breaks
  Serialize + Deserialize derive (Query is not Serialize).
• Hybrid — adapter helper takes &CteNode. Clean layering but
  same double-parse + slice-wrapper surgery as A. Strictly dominated
  by Option C.

Six-criterion rating (clean architecture, performant engineering,
idiomatic Rust, product vision, code quality, maintainability) had
Option C at 4.8/5 overall vs A at 2.2, B at 2.3, Hybrid at 3.2. Full
analysis in shape-pr-12.5-engine-api.md.

Test plan

• cargo nextest run --all-targets --locked
• cargo test --test bdd
• cargo clippy --all-targets --locked -- -D warnings
• cargo fmt --all --check
• cargo deny check
• RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items --locked
• crap4rs --coverage lcov.info (PASS, worst 8.0)
• Snapshot tests unchanged on jaffle-shop fixtures

Out of scope

• CR-1 through CR-6 (already shipped in PR a11y: DAG nodes keyboard-accessible (tabindex + role + keydown) #41 / #41 — feat(renderer): DAG keyboard accessibility + #40 renderer hygiene #49 and PR #47 — renderer: Model SQL expandable section (raw Jinja, model-level) #48).
• --config flag (cli: --config flag + AnalysisConfig (deferred from PR 6) #24), workflow hardening (infra: harden CI workflow — scope GITHUB_TOKEN, pin actions to SHA, disable checkout credential persistence #27), branch protection
  (chore: branch protection on main — require CI after first green PR cycle #16) — PR 14/15/16.

Closes #40

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@cmbays, we couldn't start this review because you've used your available PR reviews for now.

Your plan includes 1 review of capacity. Refill in 19 minutes and 24 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 87b44d61-2de2-4b28-828f-23827759cbf2

📥 Commits

Reviewing files that changed from the base of the PR and between c0313b5 and f9e22fc.

📒 Files selected for processing (4)

• ARCHITECTURE.md
• src/adapters/cte_engine.rs
• src/adapters/render.rs
• src/domain/cte.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch renderer-40-ast

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request refactors the extraction of structural facts from SQL CTEs, replacing whitespace-based heuristics in the rendering layer with a formal AST walk in the adapter layer. These facts, including whether a CTE is a simple import and its leaf table references, are now stored as POD fields on the CteNode domain object. Review feedback identified a compilation error in the AST traversal logic where a non-existent method was called on an identifier. Additionally, the reviewer provided guidance on maintaining semantic distinctions in the dependency graph when dealing with JOIN chains inside UNION arms.

[gemini-code-assist]

The sqlparser::ast::ObjectName struct in version 0.62 is a tuple struct ObjectName(pub Vec<Ident>). The Ident struct itself does not have an as_ident() method. If name.0.last() returns Some(&Ident), you can access the value field directly. This call will likely cause a compilation error.

Suggested change

	if let Some(ident) = name.0.last().and_then(|p| p.as_ident()) {
	if let Some(ident) = name.0.last() {

[cmbays]

WON'T FIX — Gemini's claim ("Ident does not have an as_ident() method, this will likely cause a compilation error") misreads the sqlparser 0.62 type structure.

Verified against the registry source (~/.cargo/registry/.../sqlparser-0.62.0/src/ast/mod.rs):

• Line 395: pub struct ObjectName(pub Vec<ObjectNamePart>); — the Vec element is ObjectNamePart, not Ident.
• Line 419: pub enum ObjectNamePart { ... }
• Line 428: impl ObjectNamePart { pub fn as_ident(&self) -> Option<&Ident> { ... } } — the canonical accessor.

So name.0.last() returns Option<&ObjectNamePart>, and .and_then(|p| p.as_ident()) resolves to Option<&Ident> — exactly the pattern used by the existing resolve_factor function in this same file (line 388 pre-PR / line 422 post-PR). Empirical confirmation: PR builds cleanly and all 293 nextest tests + 27 cucumber scenarios pass.

Applying the suggested rewrite if let Some(ident) = name.0.last() { would bind ident: &ObjectNamePart (an enum), then ident.value on the next line would fail to compile because ObjectNamePart has no value field — that lives on the Ident inside the as_ident() Option.

Logged in feedback_bot-review-suggestions-must-be-verified (this is the third PR in a row to validate that memory — character-by-character verification of bot patches is load-bearing, even on HIGH-priority findings).

[gemini-code-assist]

This function correctly identifies the 'import-CTE' shape. While this aligns with the PR's stated goal of Option C (engine-computed facts), ensure that the implementation adheres to the rule that the base relation of a JOIN chain inside a UNION arm is classified as EdgeType::From (the access role) rather than inheriting the union type (which would classify it as Transform/set-operation). This maintains the semantic distinction required for the dependency graph.

References

1. In a CTE dependency graph, the base relation of a JOIN chain inside a UNION arm should be classified as EdgeType::From (representing the access role) rather than inheriting the union type (representing the set-operation role). This maintains the semantic distinction between the access pattern and the set-operation role.

[cmbays]

Verified — no change needed. The concern (base relation of a JOIN chain inside a UNION arm must classify as EdgeType::From, not inherit the union type) applies to edge classification, which is unchanged by this PR.

The existing edges_from_join_chain at line 397+ already encodes the rule:

if table.joins.is_empty() {
    // Plain FROM reference — use union context if present, else From.
    let base_type = union_type.unwrap_or(EdgeType::From);
    push_edge(&table.relation, consumer_idx, base_type, index, edges);
} else {
    // JOIN chain: base gets From; each joined relation gets its type.
    push_edge(&table.relation, consumer_idx, EdgeType::From, index, edges);
    ...
}

The base relation gets From (not the union_type) whenever joins are present — exactly Gemini's rule. The module doc at lines 30–31 documents this: "The base relation of a JOIN chain inside a UNION arm gets From, not the union type — only join-free arm sources get the union type."

PR 12.5's is_body_simple_from_select is a top-level body shape classifier, not an edge classifier. It returns false for any body with joins (one of the three conjuncts is select.from[0].joins.is_empty()), so JOIN-inside-UNION patterns can't be misclassified as "simple-from-shape" regardless of the wrapping. The function and Gemini's concern are about orthogonal classifiers; no change here.