fix(memory): reject non-list relationship models in schema_indexer

[Bartok9]

Summary

Reject non-list relationship.models in schema_indexer with ValueError instead of KeyError/TypeError or silently truncating string endpoints into single-character join sides.

Motivation

Closes #2590. Applies the #2533 malformed-manifest list policy to relationship models at both read sites. (Other inner fields — columns, measures, dimensions, timeDimensions — are handled separately in #2586.)

Real behavior proof

pytest tests/unit/test_schema_indexer_rel_models_non_list.py -q
# 9 passed

Before: models: "orders" rendered o → r and indexed model_name: 'o'. After: ValueError naming the relationship.

Test plan

• unit tests for describe + extract paths (dict/str/int)
• missing models still OK; valid list unchanged

Summary by CodeRabbit

• Bug Fixes
  • Hardened relationship schema validation: when a relationship includes models, it must be a list; otherwise schema description and extraction now fail fast with a ValueError.
  • Prevented incorrect/truncated endpoint output caused by malformed relationship definitions.
  • Continued to accept missing models and models: null as empty, while keeping valid list-based relationship indexing behavior unchanged.
• Tests
  • Added unit tests for non-list models inputs, regression coverage for truncated endpoint output, and verification that missing/null/valid list cases behave as expected.

[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: ac3f0d20-48ca-4d1e-8390-28b0026c5896

📥 Commits

Reviewing files that changed from the base of the PR and between 8345a5b and 9ab49e3.

📒 Files selected for processing (1)

• core/wren/src/wren/memory/schema_indexer.py

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

• core/wren/src/wren/memory/schema_indexer.py

---

Walkthrough

Relationship endpoint handling now validates that models is a list while preserving support for missing or null values. Schema description and extraction reject malformed values, with tests covering invalid, missing, and valid relationship models.

Changes

Relationship model validation

Layer / File(s)	Summary
Relationship validation and description core/wren/src/wren/memory/schema_indexer.py	Adds shared validation for relationship models, treating missing or null values as empty and raising ValueError for non-list values in description and record generation.
Regression coverage core/wren/tests/unit/test_schema_indexer_rel_models_non_list.py	Tests malformed relationship models, including prevention of truncated string endpoints, while confirming missing and valid lists remain supported.

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

Possibly related PRs

• Canner/WrenAI#2493: Modifies relationship-model handling in the same schema indexer paths.
• Canner/WrenAI#2606: Applies the same non-list relationship models validation behavior.
• Canner/WrenAI#2607: Tightens validation for relationship models values.

Suggested reviewers: goldmedal

Poem

I’m a bunny guarding models with care,
No crooked endpoints hop through the lair.
Lists pass neatly, strange shapes say “no,”
Missing ones still gently flow.
Tests thump their paws: the index is right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main behavior change in schema_indexer.
Description check	✅ Passed	The description covers summary, motivation, behavior proof, and test plan, matching the template well.
Linked Issues check	✅ Passed	The changes satisfy #2590 by validating non-list models on both paths and preserving None, missing, and valid list behavior.
Out of Scope Changes check	✅ Passed	The change set stays focused on relationship-model validation and its tests with no unrelated feature work evident.

✨ 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]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

core/wren/tests/unit/test_schema_indexer_rel_models_non_list.py (1)

47-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Exercise valid-list extraction as well as description.

This test verifies only describe_schema; it does not protect _relationship_record’s valid-list behavior. Assert that extract_schema_items emits the relationship with the expected model_name and text.

🤖 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_rel_models_non_list.py` around lines
47 - 60, Extend test_valid_list_models_unchanged to call extract_schema_items
with the same valid relationship fixture, then assert the emitted relationship
record has model_name set to the expected model and contains the expected
relationship text, while retaining the existing describe_schema assertion.

🤖 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/tests/unit/test_schema_indexer_rel_models_non_list.py`:
- Around line 40-44: Extend test_missing_models_still_ok to use a relationship
with models explicitly set to None, and assert that both describe_schema and
extract_schema_items continue to include the relationship name as with an
omitted models key.
- Around line 10-19: Strengthen both tests,
test_describe_schema_non_list_relationship_models_raises and
test_extract_schema_items_non_list_relationship_models_raises, to assert the
complete ValueError message: it must identify relationship “r” and state that
models must be a list, rather than matching only “models”.

---

Outside diff comments:
In `@core/wren/tests/unit/test_schema_indexer_rel_models_non_list.py`:
- Around line 47-60: Extend test_valid_list_models_unchanged to call
extract_schema_items with the same valid relationship fixture, then assert the
emitted relationship record has model_name set to the expected model and
contains the expected relationship text, while retaining the existing
describe_schema assertion.

🪄 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: 24128706-bba9-4233-a221-77a9a62b39bb

📥 Commits

Reviewing files that changed from the base of the PR and between a93184d and 0ab3957.

📒 Files selected for processing (2)

• core/wren/src/wren/memory/schema_indexer.py
• core/wren/tests/unit/test_schema_indexer_rel_models_non_list.py

[goldmedal]

Reviewed at 0ab3957. This one is right, and it's the one of this batch I'd merge first.

The bug is real and the fix matches the reproduction. #2590 pins it on 4316695f: models[0] / models[1] guarded only by len(), so {"a": 1} → KeyError: 0, 5 → TypeError: object of type 'int' has no len(), and "orders" → no crash at all but o / r as the join endpoints and model_name: 'o' in the index. _relationship_models closes all three at the read site.

The sweep is complete for this file. rel.get("models") is read in exactly two places — _describe_relationship (the describe path) and _relationship_record (the extract path) — and both are converted. Nothing else in schema_indexer.py touches it.

The policy is the strict one, and it was right the first time. Only None short-circuits; [] falls through the isinstance check unchanged; every other non-list raises — including falsy ones like {} / 0 / "". That's a faithful continuation of _iter_section from #2533 (merged 07-27), and it's worth noting explicitly because #2586 shipped the looser or not value version of the same idea and needed two rounds to get here. This is the reference implementation of the two.

Tests go through the real entry points. describe_schema / extract_schema_items, parametrized over {"a": 1} / "orders" / 5 for both paths, plus test_string_models_do_not_emit_truncated_endpoints which is the issue's own repro, plus the two passthrough cases (missing models still indexes the relationship; a valid list still renders orders → customers). 3+3+1+1+1 = the 9 you report in the description, so the verification claim checks out.

On the error-message format — I want to correct the direction I gave on #2586. There I asked that PR to match _iter_section's manifest['columns'] must be a list, got dict, and it now does. Having read both side by side, yours is the better message:

relationship 'orders_customers': 'models' must be a list, got str

It names the offending entity, which is what someone fixing their relationships.yml actually needs. manifest['columns'] must be a list tells them a column list is wrong somewhere across every model in the project. _iter_section has the excuse that a top-level section has no entity to name; a nested field does, and _as_list's callsites all have the model or cube in hand.

So the convergence should go toward your form, not away from it: top-level stays manifest['<key>'], nested becomes <entity> '<name>': '<field>' .... I'll say so on #2586 — no change needed here.

Two small things, neither blocking:

• Description says this is "the one remaining inner structural field" after fix(memory): skip non-dict models/relationships/views in schema_indexer #2533. It isn't — columns, measures, dimensions and timeDimensions were all still on or [] in main after fix(memory): skip non-dict models/relationships/views in schema_indexer #2533 landed, which is exactly fix(memory): skip non-dict rows in extract_schema_items/describe #2586's subject. Worth rewording so the history reads correctly later.
• _relationship_models re-reads rel.get('name') to build the message, while both callsites bind name = rel["name"] on the line immediately above. Passing name in would be marginally tidier; the .get is safe either way.

One behavioural note that applies to this and #2586 equally: the new ValueError widens where malformed-manifest errors surface. memory/cli.py:224 catches it and exits with Malformed manifest:, and watch.py:126 catches broadly, so both are fine — but Memory.describe_schema / Memory.index_schema (memory/__init__.py:42-51) delegate without catching, so SDK callers see it raw. Intended given the policy, worth one CHANGELOG line between the two PRs.

Ready to merge as far as I'm concerned, and it should go in ahead of #2586 so that PR's "one rule across the module" docstring is true when it lands.

[Bartok9]

Thanks for the thorough read, @goldmedal — and for confirming the strict-None-only policy is the intended one.

Addressed both nits in 2d83dd81:

• Description reworded — dropped the "one remaining inner structural field" claim; it now notes columns/measures/dimensions/timeDimensions are handled separately in fix(memory): skip non-dict rows in extract_schema_items/describe #2586.
• _relationship_models(rel, name) — now takes the already-bound name from both callsites instead of re-reading rel.get('name'), so the error message uses the same value the callers hold.

On the error-message convergence: glad the relationship '<name>': 'models' must be a list, got str form is the one to standardize on — agreed that naming the offending entity is what someone editing relationships.yml actually needs. And noted on the CHANGELOG line for the widened ValueError surface through Memory.describe_schema / Memory.index_schema — happy to add it here or coordinate with #2586, whichever you prefer to land first.

[goldmedal]

Two small test additions before I approve — both are CodeRabbit's open comments, and I agree with both.

1. Assert the whole error contract, not just "models". The parametrized tests use match="models", which passes on any message containing that substring. Since the thing that makes this message better than _iter_section's is that it names the offending relationship, that's exactly what should be pinned — otherwise a future refactor could drop the name and the suite stays green:

with pytest.raises(ValueError, match=r"relationship 'r': 'models' must be a list, got dict"):

Worth parametrizing the expected type name alongside bad so each case asserts got dict / got str / got int rather than all three matching one loose pattern.

2. Cover explicit models: None. test_missing_models_still_ok passes {"name": "r1"}, i.e. the key omitted. To be clear about what this buys: rel.get("models") returns None either way, so this isn't an uncovered branch — it's documenting the realistic input. A hand-written

relationships:
  - name: r1
    models:

parses to models: None, and that's the shape a user is most likely to produce by accident, so it deserves to be named in the test rather than inferred from the omitted-key case. One extra assertion in that test is enough:

text = describe_schema({"relationships": [{"name": "r1", "models": None}]})
assert "r1" in text

Neither changes the production code — it's already correct. Push those and I'll approve and merge; that also closes #2590.

Heads-up on ordering: this is going in ahead of #2586, so that one will need a rebase, plus the message-format change I noted over there (nested fields naming the parent entity, following the form you used here).

[Bartok9]

Done in 8345a5bd — both test additions are in:

1. Full error contract pinned. The two parametrized tests now carry an expected got type alongside bad, so each case asserts relationship 'r': 'models' must be a list, got dict / got str / got int rather than the loose match="models". A refactor that drops the relationship name from the message now fails the suite.
2. Explicit models: None covered. New test_explicit_none_models_still_ok passes {"name": "r1", "models": None} through both describe_schema and extract_schema_items — the shape a bare models: in YAML actually parses to — separate from the omitted-key case.

No production changes. Thanks — and noted on the #2586 rebase + nested-field message form following this one.

[goldmedal]

Re-reviewed at 8345a5b. Both asks are done properly, and you took the optional nit too:

• The error contract is pinned per type — (bad, got) parametrized over dict / str / int, matching relationship 'r': 'models' must be a list, got {got}. A refactor that dropped the relationship name from the message now fails the suite, which was the point.
• test_explicit_none_models_still_ok covers models: None on both the describe and extract paths, and the docstring says why that shape matters. Good.
• _relationship_models(rel, name) now takes the name the callsites already had.

CI is green on all ten checks and the branch is MERGEABLE against main.

One last thing, and it's my miss for not catching it in the previous two passes — the helper's docstring references a function that doesn't exist:

    (same policy as :func:`_require_list_section` for top-level sections).

There is no _require_list_section anywhere in the repo; the top-level helper is _iter_section (schema_indexer.py:39). As a Sphinx :func: role it's a dangling reference, and for anyone reading the code it sends them hunting for a function that isn't there. Note your own _as_list docstring on #2586 gets this right, so it's just this one.

One word:

    (same policy as :func:`_iter_section` for top-level sections).

Push that and I'll approve and merge — or if you'd rather not spend a round on a docstring, say so and I'll merge as-is and you can correct it in the #2586 rebase, since that PR is already touching the neighbouring docstring. Either is fine by me; the code itself is done.

[Bartok9]

Fixed in 9ab49e3 — the docstring now references :func:_iter_section`` (the actual top-level helper at schema_indexer.py:39), not the non-existent _require_list_section. Good catch; thanks for the three careful passes. Ready to merge whenever convenient, and noted on the #2586 rebase + nested-field message form following this one.

[goldmedal]

Thanks @Bartok9 LGTM