fix(policy): harden resolve_model_name against non-str names

[Bartok9]

Summary

• Filter non-str model names before exact/CI matching in resolve_model_name
• Return None when the lookup name is not a non-empty string

License

Apache-2.0 (core/**).

Motivation

Corrupt or partially-migrated MDL name collections could include None/ints. Calling .lower() on those raised and aborted strict-mode planning even when a usable string candidate existed.

Verification

cd core/wren && .venv/bin/python -m pytest tests/unit/test_resolve_model_name_nonstr.py -v — 3 passed

Duplicate check

• No open PR on policy.resolve_model_name non-str filtering

Summary by CodeRabbit

• Bug Fixes

  • Improved model name resolution by safely handling invalid or empty lookup inputs.
  • Candidate model names that aren’t non-empty strings are now ignored to prevent lookup errors.
  • Preserves existing exact-match behavior for quoted model names (case-sensitive).

• Tests

  • Added unit test coverage for non-string, empty, and quoted lookup values, including scenarios that previously could raise errors.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 28a1b395-0f72-463f-b163-ed748327e44c

📥 Commits

Reviewing files that changed from the base of the PR and between ae6b43c and 26e80d8.

📒 Files selected for processing (1)

• core/wren/src/wren/policy.py

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

• core/wren/src/wren/policy.py

---

Walkthrough

resolve_model_name now rejects invalid lookup values, filters invalid candidate names, and adds tests covering non-string inputs and quoted matching behavior.

Changes

Model name resolution

Layer / File(s)	Summary
Resolver validation and coverage core/wren/src/wren/policy.py, core/wren/tests/unit/test_resolve_model_name_nonstr.py	The resolver returns None for invalid lookup names, ignores invalid candidate entries, and tests quoted exact-match behavior including casing differences.

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

Poem

I’m a bunny guarding names in a set,
Invalid strings I gently forget.
Exact case hops into view,
Wrong case gets a quiet “no.”
Safe lookups leave no regret!

🚥 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: hardening resolve_model_name against non-string model names.
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]

🧹 Nitpick comments (1)

core/wren/src/wren/policy.py (1)

162-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the redundant collection-type branch.

Both branches construct model_set with the exact same comprehension, so the set/frozenset check provides no behavioral or performance benefit.

Proposed simplification

-    if isinstance(model_names, (set, frozenset)):
-        model_set = {n for n in model_names if isinstance(n, str) and n}
-    else:
-        model_set = {n for n in model_names if isinstance(n, str) and n}
+    model_set = {n for n in model_names if isinstance(n, str) and n}

🤖 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/policy.py` around lines 162 - 165, Remove the redundant
isinstance(model_names, (set, frozenset)) branch and define model_set once using
the existing string-and-nonempty-name comprehension, preserving the current
filtering behavior for all collection types.

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

Nitpick comments:
In `@core/wren/src/wren/policy.py`:
- Around line 162-165: Remove the redundant isinstance(model_names, (set,
frozenset)) branch and define model_set once using the existing
string-and-nonempty-name comprehension, preserving the current filtering
behavior for all collection types.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f3597227-d865-4894-921b-08a40f9f7095

📥 Commits

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

📒 Files selected for processing (2)

• core/wren/src/wren/policy.py
• core/wren/tests/unit/test_resolve_model_name_nonstr.py

[Bartok9]

Good catch — collapsed the redundant set/frozenset branch into a single comprehension in 26e80d8. Behavior unchanged.

[goldmedal]

Passing on this one — it's unreachable, and it costs something on the planning hot path.

Unreachable. For a non-str entry to reach resolve_model_name, the manifest would need something like {"name": 99}. wren-core rejects that (invalid type: integer 99, expected a string) at get_manifest_extractor, a few lines later in the same try block in _plan — so the request aborts either way. The name parameter guard is dead too: the only caller passes t.name from sqlglot, and _plan already does if not t.name: continue immediately above.

Regression. The line being replaced is a deliberate fast path:

model_set = model_names if isinstance(model_names, (set, frozenset)) else set(model_names)

Callers pass a set, so today there is no copy. The patch replaces it with an unconditional set comprehension, so every call rebuilds the full name set — once per table reference, per query, scaling with the number of models in the manifest. That's a real cost in exchange for a guard that cannot fire.

Same reasoning as #2546 and #2572: manifest schema is validated once, in wren-core; I'd rather not re-validate it per callsite in Python.

[Bartok9]

Fair call — you're right that wren-core rejects a non-str name at get_manifest_extractor in the same try, so the guard can't fire, and I missed that the comprehension drops the deliberate set/frozenset fast path. Not worth a per-callsite cost on the planning path for a dead branch. Closing this one. Thanks for the detailed read.