fix(genbi): guard non-dict models/columns in inventory format

[Bartok9]

Summary

• GenBI _format_model_inventory iterated models/columns with unconditional .get.
• Non-dict MDL rows crash compose_build_instruction markdown assembly.
• Skip junk models/columns; empty inventory after filtering uses the existing empty message.

Verification

$ cd core/wren && .venv/bin/python -m pytest tests/unit/test_genbi_format_model_inventory.py -q
3 passed

Apache-2.0 path: core/wren/**.

Summary by CodeRabbit

• Bug Fixes
  • Improved model inventory formatting to be more tolerant of missing or malformed model data.
  • Invalid/non-dictionary entries and invalid column values are now ignored while preserving valid model and column names.
  • Correctly displays the “no models found” fallback when no valid models are available.
• Tests
  • Added unit test coverage for formatting valid inventories, mixed invalid inputs, and empty/invalid-only inputs.

[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: 5639f082-86f8-4bb7-bd85-dec3f25fd23d

📥 Commits

Reviewing files that changed from the base of the PR and between 081dc59 and 87bcf4d.

📒 Files selected for processing (1)

• core/wren/src/wren/genbi/composer.py

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

• core/wren/src/wren/genbi/composer.py

---

Walkthrough

_format_model_inventory now handles missing or malformed models and columns, filters invalid entries, and preserves the fallback message when no valid model bullets are produced. Unit tests cover valid, mixed, and entirely invalid inputs.

Changes

Model inventory formatting

Layer / File(s)	Summary
Defensive formatting and validation core/wren/src/wren/genbi/composer.py, core/wren/tests/unit/test_genbi_format_model_inventory.py	The formatter accepts optional input, skips invalid models and columns, normalizes malformed columns, and preserves the no-model fallback. Tests cover valid, mixed invalid, and entirely invalid input.

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

Poem

A rabbit reviewed each model with care,
Skipping the junk that was hiding in there.
Columns stood tidy, the fallback grew bright,
“No models found” waited when none were right.
Hop, test, and merge—everything’s light!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: making GenBI inventory formatting guard against non-dict models and columns.
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 (1)

core/wren/src/wren/genbi/composer.py (1)

62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify raw_cols retrieval and type check.

The or [] fallback on line 62 is redundant. If model.get("columns") returns None (or any other non-list value like a string or dictionary), the subsequent isinstance(raw_cols, list) check will evaluate to False and correctly reset raw_cols to [].

You can simplify this to a direct .get() call followed by the type check.

♻️ Proposed refactor

-        raw_cols = model.get("columns") or []
+        raw_cols = model.get("columns")
         if not isinstance(raw_cols, list):
             raw_cols = []

🤖 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/genbi/composer.py` around lines 62 - 64, In the raw_cols
retrieval logic, remove the redundant `or []` fallback from
`model.get("columns")` and keep the existing `isinstance(raw_cols, list)` check
to reset all non-list values, including None, to an empty list.

🤖 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/genbi/composer.py`:
- Around line 62-64: In the raw_cols retrieval logic, remove the redundant `or
[]` fallback from `model.get("columns")` and keep the existing
`isinstance(raw_cols, list)` check to reset all non-list values, including None,
to an empty list.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7894584d-ef3a-405e-a97b-7546e8ae4d7e

📥 Commits

Reviewing files that changed from the base of the PR and between 2b0b335 and 1b6df91.

📒 Files selected for processing (2)

• core/wren/src/wren/genbi/composer.py
• core/wren/tests/unit/test_genbi_format_model_inventory.py

[goldmedal]

Suggesting we close this one too — it's the same split as #2547 and the models half of #2567.

The model guard is dead code. _format_model_inventory has exactly one production caller: compose_build_instruction, and the only place that is called from is genbi/cli.py:143:

models=load_models(project_path),

load_models dispatches to _load_models_v1 / _load_models_v2, and both already filter their items (context.py:544, context.py:568). So if not isinstance(model, dict): continue can't be false, and neither can the all-junk [None, "x", 1] case that test_all_junk_falls_back_to_empty_message covers — that input can't be produced by the only path that reaches this function.

One line here is real. columns comes straight out of YAML and nothing upstream constrains its type, so raw_cols being a non-list is genuinely reachable and ", ".join(...) over it would break. That's worth having — but it's a two-line change to cols, not the surrounding scaffolding.

The test doesn't go through a reachable path. It calls the private _format_model_inventory directly, which is what let the unreachable inputs look meaningful. A test that goes through compose_build_instruction on a project fixture whose metadata.yml has columns: 5 would cover the real case and would have shown that the rest can't happen.

So: close, and if you want the reachable half, a focused PR guarding the columns container type — tested through compose_build_instruction — would be welcome.

[Bartok9]

Agreed — the model guard is dead code given both _load_models_v1/v2 already filter, and the test reaches it through the private function rather than a real path. The only reachable bit is the columns container type straight from YAML. Closing this, and I'll follow up with a focused two-line guard on cols tested through compose_build_instruction on a fixture whose metadata.yml has a non-list columns. Thanks for the precise callsite walk.