fix(pydantic): skip non-dict models in list_models#2564
Conversation
Corrupted MDL rows caused KeyError/TypeError in wren_list_models. Skip non-dicts and entries without name; coerce columns/properties.
Walkthrough
ChangesModel listing guards
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sdk/wren-pydantic/tests/unit/test_run_list_models_guard.py (1)
9-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMost of this stubbing is dead code for the exec-based extraction approach used below.
sys.path.insert(...)and thesys.modules[...]registrations (Lines 16-27, 32-48) are only meaningful if something actuallyimports these modules. Since the test never imports_tools(it extracts andexecs just the function body — Lines 49-70), none of thesesys.modulesentries are ever looked up except as a scratch place to grabWrenError/should_propagate/to_model_retrybefore dropping them intons. This can be simplified to defining those three values directly (e.g. plain classes/lambdas) without touchingsys.modulesorsys.pathat all.♻️ Simplification
-ROOT = Path(__file__).resolve().parents[2] -SRC = ROOT / "src" - -# Stub heavy deps before loading _tools -sys.path.insert(0, str(SRC)) - -# Provide minimal ModelSummary used by _tools -models_mod = types.ModuleType("wren_pydantic._models") - - -class ModelSummary: - def __init__(self, name, column_count, description=None): - self.name = name - self.column_count = column_count - self.description = description - - -models_mod.ModelSummary = ModelSummary -sys.modules["wren_pydantic._models"] = models_mod - -# Other imports in _tools — stub at package boundary by executing only the function -# Extract function source with ast and exec in isolation is heavy; load module with stubs. - -for name in [ - "wren", - "wren.model", - "wren.model.error", - "wren.engine", - "pydantic_ai", - "pydantic_ai.exceptions", - "wren_pydantic", - "wren_pydantic._errors", - "wren_pydantic._toolkit", -]: - sys.modules.setdefault(name, types.ModuleType(name)) - -sys.modules["wren.model.error"].WrenError = type("WrenError", (Exception,), {}) -sys.modules["wren_pydantic._errors"].should_propagate = lambda e: False -sys.modules["wren_pydantic._errors"].to_model_retry = lambda e: e +ROOT = Path(__file__).resolve().parents[2] +SRC = ROOT / "src" + + +class ModelSummary: + def __init__(self, name, column_count, description=None): + self.name = name + self.column_count = column_count + self.description = description + + +WrenError = type("WrenError", (Exception,), {}) +should_propagate = lambda e: False +to_model_retry = lambda e: e🤖 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 `@sdk/wren-pydantic/tests/unit/test_run_list_models_guard.py` around lines 9 - 48, Remove the unused sys.path insertion and sys.modules stubs from test_run_list_models_guard.py. In the exec-based extraction setup, define the required ModelSummary, WrenError, should_propagate, and to_model_retry values directly in the execution namespace, preserving the existing test behavior without package/module registration.
🤖 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 `@sdk/wren-pydantic/tests/unit/test_run_list_models_guard.py`:
- Around line 49-70: Add WrenToolkit to the ns dictionary before exec(code, ns),
using a suitable placeholder or imported symbol so _run_list_models annotation
evaluation succeeds on Python 3.11/3.12. Also replace the line-prefix extraction
with AST-based parsing to reliably isolate the _run_list_models function without
depending on top-level def/class text patterns.
---
Nitpick comments:
In `@sdk/wren-pydantic/tests/unit/test_run_list_models_guard.py`:
- Around line 9-48: Remove the unused sys.path insertion and sys.modules stubs
from test_run_list_models_guard.py. In the exec-based extraction setup, define
the required ModelSummary, WrenError, should_propagate, and to_model_retry
values directly in the execution namespace, preserving the existing test
behavior without package/module registration.
🪄 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
Run ID: 6b20b9d0-9ca5-42ec-89a5-a152b0284d6f
📒 Files selected for processing (2)
sdk/wren-pydantic/src/wren_pydantic/_tools.pysdk/wren-pydantic/tests/unit/test_run_list_models_guard.py
The previous own loads stubbed wren_pydantic._models and exec'd a slice of _tools, which caused collection NameError/ImportError for sibling unit tests. Use the real _run_list_models entrypoint with a MagicMock toolkit instead.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/wren-pydantic/tests/unit/test_run_list_models_guard.py (1)
10-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert normalization of malformed fields, not only filtering.
The test verifies that invalid rows are excluded and that valid summaries preserve derived fields, but it does not prove that a named model with non-list
columnsor non-dictpropertiesis safely converted tocolumn_count=0anddescription=None. Add explicit assertions for those cases so regressions in the new defensive normalization are caught.🤖 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 `@sdk/wren-pydantic/tests/unit/test_run_list_models_guard.py` around lines 10 - 25, The test test_skips_bad_models should explicitly verify normalization of malformed fields on the "ok" model: assert its non-list columns produce column_count=0 and its non-dict properties produce description=None, while preserving the existing assertions for the valid "t" model.
🤖 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 `@sdk/wren-pydantic/tests/unit/test_run_list_models_guard.py`:
- Around line 10-25: The test test_skips_bad_models should explicitly verify
normalization of malformed fields on the "ok" model: assert its non-list columns
produce column_count=0 and its non-dict properties produce description=None,
while preserving the existing assertions for the valid "t" model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 94bf3c45-3976-4756-a327-84009f2fdcde
📒 Files selected for processing (1)
sdk/wren-pydantic/tests/unit/test_run_list_models_guard.py
|
Closing as superseded — equivalent guard already landed on |
1 participant
Summary
Harden
_run_list_modelsso non-dict / incomplete MDL model rows do not KeyError the pydantic-ai tool.Real behavior proof
License
sdk/wren-pydantic/**Apache-2.0.Summary by CodeRabbit
Bug Fixes
Tests