Skip to content

fix(memory): reject non-list relationship models in seed queries - #2606

Open
Bartok9 wants to merge 1 commit into
Canner:mainfrom
Bartok9:fix/seed-rel-models-non-list
Open

fix(memory): reject non-list relationship models in seed queries#2606
Bartok9 wants to merge 1 commit into
Canner:mainfrom
Bartok9:fix/seed-rel-models-non-list

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

generate_seed_queries raises ValueError on non-list relationship.models instead of index/string-slice bugs.

Motivation

Same structural policy as schema_indexer (#2590 / #2533). Seeds must not invent join endpoints from a string's first characters.

Real behavior proof

pytest tests/unit/test_seed_queries.py -q -k RelationshipSeed
# 2 passed

Test plan

  • str/dict models raise; suite otherwise unchanged

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation for relationship model configurations.
    • Clear errors are now shown when model values use an unsupported format.
    • Explicitly empty model configurations are handled correctly.
  • Tests

    • Added coverage for invalid string and object model values.

@github-actions github-actions Bot added python Pull requests that update Python code core labels Jul 28, 2026
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

_relationship_seed now treats relationships[].models: null as empty and raises ValueError for non-list values. Unit tests cover string and dictionary inputs.

Changes

Relationship models validation

Layer / File(s) Summary
Validate relationship models input
core/wren/src/wren/memory/seed_queries.py, core/wren/tests/unit/test_seed_queries.py
_relationship_seed normalizes null values, rejects non-list models values with a descriptive ValueError, and tests string and dictionary cases.

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

Possibly related PRs

Suggested reviewers: goldmedal

Poem

I’m a rabbit guarding the models list,
Null becomes an empty twist.
Strings and dicts now raise alarm,
Tests keep the seed path safe from harm.
Hop, hop—validation is calm!

🚥 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: validating relationship models in seed queries and rejecting non-list values.
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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
core/wren/tests/unit/test_seed_queries.py (1)

819-842: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for models: null.

The implementation intentionally treats None as an empty list, but these tests cover only rejected string and dictionary values. Add a case asserting that a relationship with models=None produces no relationship seed.

🤖 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_seed_queries.py` around lines 819 - 842, Add a unit
test to TestRelationshipSeedModelsType covering a relationship whose models
value is None, and assert generate_seed_queries returns no relationship seed for
that manifest. Keep the existing rejection tests for string and dictionary
values unchanged.
🤖 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/tests/unit/test_seed_queries.py`:
- Around line 819-842: Add a unit test to TestRelationshipSeedModelsType
covering a relationship whose models value is None, and assert
generate_seed_queries returns no relationship seed for that manifest. Keep the
existing rejection tests for string and dictionary values unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b9bf5ecf-889e-48ca-a1cd-e37bc32fdf61

📥 Commits

Reviewing files that changed from the base of the PR and between a93184d and 8ad4ab1.

📒 Files selected for processing (2)
  • core/wren/src/wren/memory/seed_queries.py
  • core/wren/tests/unit/test_seed_queries.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant