fix(mdl): skip non-dict models/columns/views in CTERewriter

[Bartok9]

Summary

• Skip non-dict / nameless models, columns, and views when building CTERewriter indexes
• Harden _iter_model_column_names and case-collision scans the same way

License

Apache-2.0 (core/**).

Motivation

A single non-object row in manifest.models / columns / views raised TypeError / KeyError during rewriter construction and blocked planning for the whole catalog. Match defensive guards already used in memory/mcp formatters.

Verification

cd core/wren && .venv/bin/python -m pytest tests/unit/test_cte_rewriter_nonduct.py -v

— 2 passed

Duplicate check

• No open PR targeting CTERewriter non-dict model/view gates (engine name collection is fix(engine): skip non-dict models/views when collecting names #2546)

Summary by CodeRabbit

• Bug Fixes

  • Improved resilience when processing malformed MDL manifests.
  • Invalid models, views, and column definitions are now safely ignored instead of causing processing errors.
  • Existing handling for case-only column name collisions is preserved.

• Tests

  • Added coverage for malformed manifest entries and invalid column definitions.

[coderabbitai]

Walkthrough

Changes

CTE manifest validation

Layer / File(s)	Summary
Manifest entry parsing core/wren/src/wren/mdl/cte_rewriter.py, core/wren/tests/unit/test_cte_rewriter_nonduct.py	CTERewriter validates model, view, and column structures, skips malformed entries, and tests retained names and columns.
Collision scan hardening core/wren/src/wren/mdl/cte_rewriter.py, core/wren/tests/unit/test_cte_rewriter_nonduct.py	Case-distinct column scanning and collision reporting tolerate invalid models and columns, with iterator coverage for malformed column items.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• Canner/WrenAI#2533: Hardens analogous MDL model/view and column parsing.
• Canner/WrenAI#2540: Adds defensive filtering in a related schema path.
• Canner/WrenAI#2400: Modifies related CTERewriter collision handling.

Suggested reviewers: goldmedal

Poem

I nibbled malformed manifests with care,
Skipping bad shapes floating there.
Good names and columns stayed in the queue,
While collision checks grew sturdy too.
Hop, hop—tests now guard the view!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: CTERewriter now skips invalid models, columns, and views.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

core/wren/tests/unit/test_cte_rewriter_nonduct.py (1)

26-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for nameless models in the collision scan.

This test covers malformed entries but does not include a dict model without a valid name containing case-distinct visible columns. Add that case using a case-insensitive dialect and assert construction skips the malformed model without raising.

🤖 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 `@core/wren/tests/unit/test_cte_rewriter_nonduct.py` around lines 26 - 54,
Extend test_skips_nonduct_models_and_views with a dict model whose name is
missing or invalid and whose visible columns differ only by case. Construct the
rewriter using a case-insensitive dialect, then assert malformed-model
construction completes without raising and the nameless model is excluded from
the indexed model data.

🤖 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 `@core/wren/src/wren/mdl/cte_rewriter.py`:
- Around line 224-226: Update both case-collision scans in the relevant rewriter
logic to skip model dictionaries unless their name is a non-empty string,
matching the initialization filter. Store the validated name locally and use it
when calling or formatting _raise_case_collision, avoiding direct model['name']
access for discarded models.

---

Nitpick comments:
In `@core/wren/tests/unit/test_cte_rewriter_nonduct.py`:
- Around line 26-54: Extend test_skips_nonduct_models_and_views with a dict
model whose name is missing or invalid and whose visible columns differ only by
case. Construct the rewriter using a case-insensitive dialect, then assert
malformed-model construction completes without raising and the nameless model is
excluded from the indexed model data.

🪄 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: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 36945b78-99b9-4134-8534-d9b31017d180

📥 Commits

Reviewing files that changed from the base of the PR and between d472877 and 3e0161b.

📒 Files selected for processing (2)

• core/wren/src/wren/mdl/cte_rewriter.py
• core/wren/tests/unit/test_cte_rewriter_nonduct.py

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Apply the valid-name filter to collision scans.

Initialization skips models whose name is missing, empty, or non-string, but the case-collision scans still process those dicts. A discarded model containing visible Year/year columns can therefore activate case-sensitive mode, or reach _raise_case_collision() and fail at model['name'] with KeyError.

Use the same name predicate in both scans and interpolate the validated local name in the error message.

Proposed fix

 for model in self.manifest.get("models", []):
     if not isinstance(model, dict):
         continue
+    model_name = model.get("name")
+    if not isinstance(model_name, str) or not model_name:
+        continue

-                            f"Model '{model['name']}' has columns that differ "
+                            f"Model '{model_name}' has columns that differ "

Also applies to: 242-244

🤖 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 `@core/wren/src/wren/mdl/cte_rewriter.py` around lines 224 - 226, Update both
case-collision scans in the relevant rewriter logic to skip model dictionaries
unless their name is a non-empty string, matching the initialization filter.
Store the validated name locally and use it when calling or formatting
_raise_case_collision, avoiding direct model['name'] access for discarded
models.

[goldmedal]

Passing on this one too, for the same reason as #2546 — but the argument is actually stronger here.

CTERewriter has exactly one production construction site, engine.py:222, and the line immediately before it is get_session_context(effective_manifest, ...) — same manifest string, same try block. Every shape this PR guards against (null / string / int rows, missing or non-str name) is rejected by wren-core's serde deserialization at that call, so the rewriter is never constructed with a junk manifest.

On top of that, in the normal path effective_manifest is not user input at all: it is extractor.extract_by(tables) re-serialized via to_json_base64(), i.e. output produced by wren-core from an already-validated Rust Manifest. A non-dict model row there would mean wren-core emitted invalid JSON of its own.

So the guards can't fire, and the tests only exercise the guarded functions directly rather than any reachable path. I'd rather keep the invariant explicit — manifest schema is validated once, in wren-core — than add per-callsite re-validation that implies otherwise.

[Bartok9]

Agreed — with get_session_context(effective_manifest, ...) on the line right before construction, and effective_manifest being wren-core output in the normal path, every shape guarded here is already rejected upstream. The invariant is better kept explicit in one place. Closing. Thanks for walking through it.