fix(memory): skip non-dict columns in schema_indexer - #2515
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSchema indexing now skips malformed or unnamed manifest entries during schema description and item extraction, with unit tests covering invalid models, views, relationships, and columns. ChangesSchema Column Hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
core/wren/src/wren/memory/schema_indexer.py (1)
90-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFilter columns before appending the header to avoid a dangling section.
If the
columnslist only contains malformed items (e.g.,[None]),if cols:will still evaluate to true. This results in the" Columns:"header being appended to the schema description, followed by nothing because all items are skipped in the loop.Consider pre-filtering the columns just like in
_model_recordso that the header is only added when valid columns exist.♻️ Proposed refactor
- cols = model.get("columns", []) + cols = [c for c in (model.get("columns") or []) if isinstance(c, dict) and c.get("name")] if cols: lines.append(" Columns:") for col in cols: - if not isinstance(col, dict): - continue _describe_column(col, lines)🤖 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/memory/schema_indexer.py` around lines 90 - 96, Filter the columns collection in the schema description flow before the `if cols` check, retaining only dictionary entries as `_model_record` does. Use the filtered collection for both the header condition and iteration so `" Columns:"` is emitted only when at least one valid column exists.
🤖 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/memory/schema_indexer.py`:
- Around line 90-96: Filter the columns collection in the schema description
flow before the `if cols` check, retaining only dictionary entries as
`_model_record` does. Use the filtered collection for both the header condition
and iteration so `" Columns:"` is emitted only when at least one valid column
exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e823f201-d6d6-4b5e-bd0a-7537cb28ed05
📒 Files selected for processing (2)
core/wren/src/wren/memory/schema_indexer.pycore/wren/tests/unit/test_schema_indexer_malformed_columns.py
CI lint failed on ruff format --check for schema_indexer.py.
Address CodeRabbit nitpick: filter columns before appending the 'Columns:' header so a list of only malformed entries doesn't emit a dangling header. Mirrors _model_record's filtering approach.
|
Thanks @coderabbitai — applied the pre-filter so the |
|
Tip For best results, initiate chat on the files or code changes.
(๑˃̵ᴗ˂̵)و |
|
Nice, minimally-invasive fix and the direction is right. CI is green ( I verified the behaviour by running the PR head directly. A few actionable items before merge: 1. The
|
| input | current behaviour |
|---|---|
{"name": ""} (empty-string name) |
correctly skipped |
columns: null |
handled, no crash |
non-dict scalar, e.g. "amount" |
correctly skipped |
6. Please squash on merge
Two of the four commits are style: ruff-format fixups, and style isn't in the project's conventional-commit set (feat / fix / chore / refactor / test / docs / perf / deps). The PR title is fix(memory): … and the title validator passed, so a squash merge is fine — but a rebase merge would put style: commits in front of release-please.
Unrelated, but noticed while checking: the lint job only runs ruff format --check src/ and ruff check src/, so files under tests/ aren't format-checked.
No blockers. I'd consider 1 and 3 worth doing before merge; 2 just needs a call on whether to widen the fix or narrow the description.
…tests - extend isinstance/name guards to models/relationships/views in describe_schema and extract_schema_items (null slots no longer crash) - drop unreachable ValueError in _column_record; caller guarantees name - add tests: dangling Columns header, empty-string/null-column, null model/rel/view slots, scalar columns; tighten weak substring assertion
|
Thanks for the thorough pass @goldmedal — all addressed in 8730311:
CI green, tests still pass (7 in the malformed-columns module). |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/wren/tests/unit/test_schema_indexer_malformed_columns.py (1)
47-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover nameless relationship/view entries, not only null slots.
This test does not catch
{}or{"name": ""}entries, and it does not exerciseextract_schema_items(). Add those cases and assert that no relationship/view sections or records are produced.🤖 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_schema_indexer_malformed_columns.py` around lines 47 - 50, Add coverage in test_describe_schema_skips_null_model_relationship_view_slots for nameless relationship and view entries, including {} and {"name": ""}, and exercise extract_schema_items() directly. Assert these entries produce no relationship or view sections or records, while preserving the existing null-slot coverage.
🤖 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/memory/schema_indexer.py`:
- Around line 57-59: Update the relationship and view processing paths to
require each dictionary entry to contain a truthy name before invoking
_describe_relationship, the corresponding view helper, or record builders. Skip
empty dictionaries and entries with blank or missing names while preserving
existing handling for valid named entries. Add regression coverage for nameless
relationships and views.
---
Nitpick comments:
In `@core/wren/tests/unit/test_schema_indexer_malformed_columns.py`:
- Around line 47-50: Add coverage in
test_describe_schema_skips_null_model_relationship_view_slots for nameless
relationship and view entries, including {} and {"name": ""}, and exercise
extract_schema_items() directly. Assert these entries produce no relationship or
view sections or records, while preserving the existing null-slot coverage.
🪄 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: b61c236d-21f7-43bf-92e5-9ede90f6dff2
📒 Files selected for processing (2)
core/wren/src/wren/memory/schema_indexer.pycore/wren/tests/unit/test_schema_indexer_malformed_columns.py
| for rel in manifest.get("relationships", []): | ||
| _describe_relationship(rel, lines) | ||
| if isinstance(rel, dict): | ||
| _describe_relationship(rel, lines) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skip nameless relationships and views in both paths.
These guards reject non-dictionaries but still pass {} or {"name": ""} to the relationship/view helpers and record builders, contrary to the PR’s stated contract. Require a truthy name here, and add regression cases for nameless relationships/views.
Proposed fix
for rel in manifest.get("relationships", []):
- if isinstance(rel, dict):
+ if isinstance(rel, dict) and rel.get("name"):
_describe_relationship(rel, lines)
for rel in manifest.get("relationships", []):
- if not isinstance(rel, dict):
+ if not isinstance(rel, dict) or not rel.get("name"):
continue
for view in manifest.get("views", []):
- if not isinstance(view, dict):
+ if not isinstance(view, dict) or not view.get("name"):
continueAlso applies to: 247-255
🤖 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/memory/schema_indexer.py` around lines 57 - 59, Update the
relationship and view processing paths to require each dictionary entry to
contain a truthy name before invoking _describe_relationship, the corresponding
view helper, or record builders. Skip empty dictionaries and entries with blank
or missing names while preserving existing handling for valid named entries. Add
regression coverage for nameless relationships and views.
|
Thanks for the follow-up — items 1–6 from the last round all look correctly applied, and CI is green across all 9 checks. However, I don't think this should merge as-is: it's been superseded by #2533, which merged into I compared this branch's head ( Cases still broken here, already fixed on The relationship guard here is No behavioural delta: all 7 tests from this branch pass unmodified against Rebase risk: this branch is Suggest closing this as superseded, and opening a small follow-up that cherry-picks the one genuinely additive test: |
|
Agreed on all counts @goldmedal — thanks for taking the time to diff head-vs-main so carefully. #2533 is the strict superset (adds I'll open a small follow-up that cherry-picks the one genuinely additive case — |
2 participants
Summary
describe_schema/extract_schema_items/_model_recordskip non-dict or nameless column entries instead of TypeError.core/wren/**.Motivation
Hand-edited MDL manifests can contain null column slots. Schema indexing should tolerate them and still index valid columns.
Verification
cd core/wren && .venv/bin/python -m pytest tests/unit/test_schema_indexer_malformed_columns.py -v— 2 passedReal behavior proof
Both tests PASSED (describe + extract paths).
Summary by CodeRabbit