fix(context): harden description checks for bad MDL rows - #2616
Conversation
_check_descriptions assumed models/views/columns/properties were always well-typed. Skip non-dict rows so validation cannot crash the CLI.
WalkthroughDescription validation now safely handles malformed model, view, column, and property structures. A unit test verifies that invalid entries are skipped while missing descriptions still produce expected warnings. ChangesDescription validation guards
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 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)
core/wren/tests/unit/test_check_descriptions_guards.py (1)
4-23: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest the public validation path and collection-shape guards.
This test invokes
_check_descriptionsdirectly, so it cannot catch the unsafevalidate_manifestdry-plan loop for malformed views. Add a regression throughvalidate_manifestwith a non-dictionary view, plus non-listmodelsandviewscases.🤖 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_check_descriptions_guards.py` around lines 4 - 23, Add regression coverage through the public validate_manifest path rather than calling _check_descriptions directly. Include malformed manifests with a non-dictionary view and non-list models and views, and assert validation completes safely with the expected result; preserve coverage for malformed model and column entries where applicable.
🤖 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/context.py`:
- Around line 1591-1596: Normalize and filter raw view entries in
validate_manifest before the dry-planning logic, ensuring non-list collections
and non-dictionary rows are safely ignored without crashing. Update
core/wren/tests/unit/test_check_descriptions_guards.py to cover
validate_manifest through its public entry point with malformed views and
non-list view collections.
---
Nitpick comments:
In `@core/wren/tests/unit/test_check_descriptions_guards.py`:
- Around line 4-23: Add regression coverage through the public validate_manifest
path rather than calling _check_descriptions directly. Include malformed
manifests with a non-dictionary view and non-list models and views, and assert
validation completes safely with the expected result; preserve coverage for
malformed model and column entries where applicable.
🪄 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: ed80ee3f-e825-4bcc-85ff-af5dd8450012
📒 Files selected for processing (2)
core/wren/src/wren/context.pycore/wren/tests/unit/test_check_descriptions_guards.py
| views = manifest.get("views", []) or [] | ||
| if not isinstance(views, list): | ||
| views = [] | ||
| for view in views: | ||
| if not isinstance(view, dict): | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Malformed view rows can still crash the public validator.
The helper-level guards are correct, but validate_manifest processes raw views before reaching _check_descriptions. Fix the public dry-plan path and add regression coverage through that entry point.
core/wren/src/wren/context.py#L1591-L1596: normalize/filter malformed views before dry-planning.core/wren/tests/unit/test_check_descriptions_guards.py#L4-L23: exercisevalidate_manifest, including malformed views and non-list collections.
📍 Affects 2 files
core/wren/src/wren/context.py#L1591-L1596(this comment)core/wren/tests/unit/test_check_descriptions_guards.py#L4-L23
🤖 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/context.py` around lines 1591 - 1596, Normalize and filter
raw view entries in validate_manifest before the dry-planning logic, ensuring
non-list collections and non-dictionary rows are safely ignored without
crashing. Update core/wren/tests/unit/test_check_descriptions_guards.py to cover
validate_manifest through its public entry point with malformed views and
non-list view collections.
1 participant
Summary
Description lint crashed on non-dict models/views/columns or non-dict
properties.Verification
Summary by CodeRabbit
Bug Fixes
Tests