fix(engine): skip non-dict models/views when collecting names

[Bartok9]

Summary

• WrenEngine._plan built model_names / view_names with m["name"] over raw manifest_json["models"|"views"].
• Non-dict or nameless entries (hand-edited/partial MDL) raised TypeError/KeyError before policy checks.
• Introduce _named_manifest_entries and use it for both lists.

Motivation

Same defensive pattern as memory schema_indexer / seed_query guards already on Apache-2.0 core/** paths. Planning should degrade to empty name sets for junk rows, not abort the request.

Verification

$ cd core/wren && .venv/bin/python -m pytest tests/unit/test_engine_manifest_names.py -q
3 passed

Real behavior proof

>>> # before: m["name"] on None → TypeError
>>> models = [{"name": "orders"}, None, "x"]
>>> {m["name"] for m in models}
TypeError
>>> from wren.engine import _named_manifest_entries
>>> _named_manifest_entries(models)
{'orders'}

Apache-2.0 path: core/wren/** only.

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of manifest model/view entries when they are missing, malformed, or contain invalid/empty names.
  • Planning and policy validation now skips bad entries instead of failing on unexpected manifest data.

• Tests

  • Added unit coverage to ensure invalid inputs (null, empty, non-list, non-dict, and nameless entries) are safely ignored for both models and views.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b959c95f-493e-45b9-b3fb-d3979c4e633a

📥 Commits

Reviewing files that changed from the base of the PR and between 90c1fa1 and 871406d.

📒 Files selected for processing (2)

• core/wren/src/wren/engine.py
• core/wren/tests/unit/test_engine_manifest_names.py

🚧 Files skipped from review as they are similar to previous changes (2)

• core/wren/tests/unit/test_engine_manifest_names.py
• core/wren/src/wren/engine.py

---

Walkthrough

Manifest planning now safely extracts non-empty names from valid model and view entries, ignoring malformed manifest data. Unit tests cover invalid entries, empty inputs, non-list inputs, and view handling.

Changes

Manifest name handling

Layer / File(s)	Summary
Safe manifest name collection core/wren/src/wren/engine.py, core/wren/tests/unit/test_engine_manifest_names.py	Adds guarded manifest-name extraction, integrates it into model and view planning, and tests malformed and empty inputs.

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

Poem

A bunny checked each manifest line,
Skipped broken names in a tidy design.
Models and views now safely hop,
Empty lists make failures stop.
Tests wave carrots: all is fine!

🚥 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 clearly matches the main change: making engine name collection skip invalid model and view entries.
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.}

[goldmedal]

Thanks for the patch, but I'm going to pass on this one.

The shapes this guards against are already rejected by wren-core's serde deserialization ~25 lines later, inside the same try block. I checked each variant against the installed wren_core:

manifest shape	{m["name"] for m in ...}	ManifestExtractor
models: [..., null]	TypeError	rejects — invalid type: null, expected struct Model
models: [..., "bad"]	TypeError	rejects — invalid type: string
model missing name	KeyError	rejects — missing field `name`
name: 99	(accepted)	rejects — expected a string
models: null / {}	(empty set)	rejects — expected a sequence
views: [null]	TypeError	rejects — expected struct View
name: ""	(accepted)	accepts

So the request aborts either way — _plan already catches the TypeError and converts it to a WrenError. The only observable change is the error text in strict mode ('NoneType' object is not subscriptable → the serde message). That's a nicer message, but it isn't what the PR description claims ("degrade to empty name sets ... not abort the request") — planning still aborts at get_manifest_extractor.

The broader reason I'd rather not take it: manifest schema is validated once, in wren-core. Adding per-callsite guards in Python inverts that invariant into "any code reading the manifest must re-validate it", and there are ~15 other places that read manifest.get("models"). Merging this implies the same guard belongs in all of them — which is what #2572, #2567 and #2573 propose. I'd rather keep the single validation boundary.

If you'd like to fix the symptom that's actually visible here, there is a real one worth doing: a corrupt mdl.json currently surfaces as ErrorCode.INVALID_SQL with phase=SQL_PLANNING, i.e. a broken MDL file is reported to the user as a broken SQL query. A focused fix would either validate the manifest once in _load_manifest() and fail fast, or map JSON/serde failures in _plan's except to an MDL-specific error code. That's one place rather than fifteen, and it changes something a user can see. Happy to review a PR along those lines.

[Bartok9]

Thanks for the detailed trace — that's a convincing analysis. You're right: wren-core's serde deserialization already rejects every malformed shape ~25 lines later inside the same try, so planning aborts either way and the only real delta is a nicer error string in strict mode. Guarding per-callsite does invert the "validate once at the wren-core boundary" invariant across ~15 read sites, which isn't a good trade.

Closing this. The corrupt-mdl.json-surfacing-as-INVALID_SQL/SQL_PLANNING symptom is the one worth fixing — I'll look at validating once in _load_manifest() (fail fast) or mapping JSON/serde failures in _plan's except to an MDL-specific error code, and open a focused PR along those lines. Appreciate the steer.