fix(context): guard wren context show/validate against null relationship models

[AnnaSuSu]

Summary

• Guard the two relationship readers on the wren context CLI surface against an explicit YAML/JSON null for a relationship's models: validate_project (context.py) and the show summary display (context_cli.py).
• User-facing impact: wren context validate and wren context show no longer abort with an unhandled TypeError when a project's relationships.yml contains a relationship whose models: is left blank (YAML null).

Motivation

Both readers do the equivalent of:

ref_models = rel.get("models", [])   # validate_project
" ↔ ".join(r.get("models", []))      # show summary

dict.get(key, default) only substitutes the default when the key is absent, not when its value is None. load_relationships reads relationships.yml and passes each entry straight through, so a relationship written as

relationships:
  - name: rel1
    models:            # YAML null
    condition: a.x = b.y

reaches these readers with models = None, crashing for m in None (validate) and " ↔ ".join(None) (show).

This is the same null-slip the maintainer already fixed in the memory package in #2424 (seed_queries._relationship_seed, with test_relationship_null_models_skipped); this PR closes the same gap on the context command surface. (A companion PR closes it in wren.memory.schema_indexer.)

Fix

Use an or fallback at both sites: rel.get("models") or []. Behaviour is unchanged for every non-null input; only the crashing null case changes — validate treats it as a relationship with no listed models (consistent with how it already handles an empty models list), and show prints an empty endpoint list.

Verification

• Two new regression tests drive the real CLI via Typer's CliRunner:
  • test_validate_tolerates_null_relationship_models
  • test_show_tolerates_null_relationship_models
  • Without the change: TypeError: 'NoneType' object is not iterable (validate) and TypeError: can only join an iterable (show) — RED.
  • With the change: both PASS.
• Per-site red check — each guard is independently load-bearing: reverting context.py alone reds the validate test only; reverting context_cli.py alone reds the show test only.
• Full unit suite: uv run --no-sync pytest tests/unit/ --ignore=tests/unit/test_memory.py → 947 passed, 2 skipped (baseline 945 + the two new tests; 0 regressions).
• Lint: just lint clean.
• End-to-end via the real wren CLI: wren context show prints rel1 (, MANY_TO_ONE) and exits 0; wren context validate runs to completion — neither raises.

Scope

Two reachable sites, both fed by the user-authored relationships.yml. The show --from-osi summary (_show_from_osi, context_cli.py) has the identical expression but is not reachable with a null models: build_manifest_from_osi always constructs relationship models as [src, dst] (and drops relationships it can't build), so no null reaches it — left untouched per "don't guard a case that can't occur." The dbt.py relationship reader builds its relationships internally rather than from an untrusted manifest and is likewise not reached.

License

core/** is Apache-2.0 per the repo LICENSE path map — this contribution is within an Apache-2.0 path.

Summary by CodeRabbit

• Bug Fixes

  • Project validation now safely handles relationship models entries that are present but explicitly null.
  • The context summary command now renders relationship model lists without crashing when models is null.

• Tests

  • Added unit/integration coverage for both context validate and context show to confirm correct behavior with null relationship model lists.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2da1ab94-5776-4bbc-9eec-c6f26e93853e

📥 Commits

Reviewing files that changed from the base of the PR and between cae3d42 and d54f408.

📒 Files selected for processing (1)

• core/wren/tests/unit/test_context_cli.py

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

• core/wren/tests/unit/test_context_cli.py

---

Walkthrough

Relationship validation and wren context show now treat explicit null models values as empty lists. CLI tests cover both commands with relationships containing models: null.

Changes

Null relationship models

Layer / File(s)	Summary
Normalize null models and test CLI commands core/wren/src/wren/context.py, core/wren/src/wren/context_cli.py, core/wren/tests/unit/test_context_cli.py	Validation and relationship rendering safely handle null models values, with tests covering context validate and context show.

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

Possibly related PRs

• Canner/WrenAI#2424: Handles the same null relationship.models shape during seed-query generation.

Poem

A bunny found null where model names should be,
So empty lists hopped into place with glee.
Validate runs smooth, show prints bright,
Tests guard the path both day and night.
“No TypeError!” the rabbit sings,
While safe defaults sprout new wings.

🚥 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 summarizes the main fix: guarding context show/validate against null relationship models.
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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (2)

core/wren/src/wren/context_cli.py (1)

1209-1209: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Apply the same null-safe fallback in _show_from_osi.

Line 1209 still uses r.get("models", []), which returns None (not []) when models is present but null — the exact bug this PR fixes in the other two readers. If an OSI-generated manifest ever contains a relationship with null models, this path will raise TypeError on " ↔ ".join(None).

♻️ Proposed fix for consistency

-            models_str = " ↔ ".join(r.get("models", []))
+            models_str = " ↔ ".join(r.get("models") or [])

🤖 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_cli.py` at line 1209, Update the models extraction
in `_show_from_osi` to fall back to an empty list when `r.get("models")` returns
null, ensuring the subsequent `" ↔ ".join(...)` remains safe for OSI manifests
with null models.

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

477-488: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Strengthen assertions to check exit code, not just TypeError absence.

Both tests only assert that result.exception is not a TypeError. A different failure (e.g., a different exception or a nonzero exit code) would pass silently. Adding assert result.exit_code == 0 ensures the commands actually succeed.

💚 Proposed fix to strengthen test assertions

 def test_validate_tolerates_null_relationship_models(tmp_path):
     _make_valid_project(tmp_path)
     _write_null_models_relationship(tmp_path)
     result = runner.invoke(app, ["context", "validate", "--path", str(tmp_path)])
-    assert not isinstance(result.exception, TypeError), result.output
+    assert result.exit_code == 0, result.output


 def test_show_tolerates_null_relationship_models(tmp_path):
     _make_valid_project(tmp_path)
     _write_null_models_relationship(tmp_path)
     result = runner.invoke(app, ["context", "show", "--path", str(tmp_path)])
-    assert not isinstance(result.exception, TypeError), result.output
+    assert result.exit_code == 0, result.output

🤖 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_context_cli.py` around lines 477 - 488, Strengthen
test_validate_tolerates_null_relationship_models and
test_show_tolerates_null_relationship_models by asserting result.exit_code == 0,
while retaining the existing TypeError assertion and diagnostic output. Ensure
both CLI commands are verified to complete successfully.

🤖 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/context_cli.py`:
- Line 1209: Update the models extraction in `_show_from_osi` to fall back to an
empty list when `r.get("models")` returns null, ensuring the subsequent `" ↔
".join(...)` remains safe for OSI manifests with null models.

In `@core/wren/tests/unit/test_context_cli.py`:
- Around line 477-488: Strengthen
test_validate_tolerates_null_relationship_models and
test_show_tolerates_null_relationship_models by asserting result.exit_code == 0,
while retaining the existing TypeError assertion and diagnostic output. Ensure
both CLI commands are verified to complete successfully.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4e7a0096-95f2-43d4-9426-75a2a481d551

📥 Commits

Reviewing files that changed from the base of the PR and between bb35d54 and cae3d42.

📒 Files selected for processing (3)

• core/wren/src/wren/context.py
• core/wren/src/wren/context_cli.py
• core/wren/tests/unit/test_context_cli.py

[AnnaSuSu]

@coderabbitai Thanks — took one of the two, and here is why the other one does not apply.

_show_from_osi (context_cli.py:1209) — not changing. That reader is not fed a user-authored manifest. Its manifest comes from build_manifest_from_osi(), and osi.py:574 constructs each relationship itself:

rel_dict: dict = {
    "name": name,
    "models": [src, dst],
    ...
}

models is always a freshly built two-element list, and _convert_relationship only appends the dict when it is truthy — so r.get("models") cannot be None on that path. The two sites this PR does fix (context.py:1158, context_cli.py:760) read the user's relationships.yml / mdl.json, where models: can be an explicit YAML null. Guarding a value we construct ourselves would be defensive code for an unreachable state, so I would rather leave the asymmetry visible than paper over it.

Test assertions — fixed in d54f408. You were right that assert not isinstance(result.exception, TypeError) lets any other failure mode pass silently. But exit_code == 0 is only correct for one of the two commands; I checked both against the real behaviour:

• wren context show → exit 0, no exception. Now asserted as result.exit_code == 0.
• wren context validate → exit 1. Once the TypeError is gone, the null-models manifest reaches the engine, which rejects it ([INVALID_SQL] Serde JSON error: invalid type: null, expected a sequence) and validate reports it as a semantic error. That is the intended outcome — a reported validation failure instead of a traceback — so the test now pins isinstance(result.exception, SystemExit) (i.e. typer.Exit, not a crash) plus exit_code == 1.

Both tests were re-checked by reverting each guard independently: reverting context.py reddens only the validate test, reverting context_cli.py reddens only the show test.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
fd -a osi.py core/wren/src
echo "---"
rg -n "rel_dict" -A15 core/wren/src/wren/osi.py | head -60
echo "---convert_relationship---"
rg -n "_convert_relationship" -A20 core/wren/src/wren/osi.py | head -60
echo "---context_cli show_from_osi---"
rg -n "_show_from_osi" -A30 core/wren/src/wren/context_cli.py | head -60

Length of output: 4781

---

@AnnaSuSu Thanks for digging in and verifying against the code — I checked osi.py and context_cli.py myself and your reasoning holds up:

• _convert_relationship (osi.py Line 512) only returns a non-empty dict when both from/to are valid strings, and the caller (osi.py Line 721) only appends via if rel:, so any relationship reaching manifest always carries "models": [src, dst] (osi.py Line 574) — a freshly built two-element list, never None.
• Since _show_from_osi (context_cli.py Line 1147) sources its manifest exclusively from build_manifest_from_osi(), that code path can never see a None models value. Adding a guard there would indeed be defensive code for a state that's structurally unreachable, so leaving the asymmetry as-is makes sense.

And the refined test assertions in d54f408 are a better fit than my original suggestion — pinning exit_code == 0 for show and isinstance(result.exception, SystemExit) + exit_code == 1 for validate correctly distinguishes "reported semantic failure" from "unhandled crash," which is the actual intended behavior once the TypeError is gone. Nice catch verifying both independently by reverting each guard.

(๑˘•ᴗ•˘)ﻭ

[goldmedal]

👍