fix(memory): skip non-dict rows in extract_schema_items/describe (#2586)

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

@@ -23,6 +23,40 @@ def manifest_hash(manifest: dict) -> str:
     return hashlib.sha256(raw.encode()).hexdigest()[:16]
 
 
+def _as_list(value: object, kind: str, name: str, field: str) -> list:
+    """Return ``value`` as a list, raising on any non-list, non-null value.
+
+    Nested collection fields (columns, measures, dimensions, timeDimensions)
+    use this helper. Only a missing/null value is treated as an empty list;
+    an empty list stays ``[]`` via the ``isinstance`` check below. Any other
+    non-list value — truthy (``columns: 42``) or falsy (``columns: {}``,
+    ``columns: 0``, ``columns: ""``) — is a structural error in the manifest,
+    so we raise ``ValueError`` rather than silently indexing a model with zero
+    columns. This matches :func:`_iter_section` for top-level sections and the
+    same-file ``_relationship_models`` policy (see #2605, now on ``main``): one
+    rule across the module — ``None`` passes, everything non-list raises. Nested
+    fields name the offending entity (``model 'orders': 'columns' must be a
+    list, got dict``) so the diagnosis points at one row rather than every model
+    in the project; top-level sections keep the ``manifest['models']`` form.
+    Cubes are not required to have a name, so an empty name falls back to a
+    ``cube (unnamed): 'measures'`` form rather than a top-level
+    ``manifest['measures']`` form: ``measures`` is never a top-level manifest
+    key, so pointing there would send the reader hunting for something that
+    does not exist. ``(unnamed)`` keeps the message truthful about where the
+    field lives while signalling that the entity could not be identified. The
+    CLI already catches ``ValueError`` and exits with ``Malformed manifest:
+    {e}``.
+    """
+    if value is None:
+        return []
+    if not isinstance(value, list):
+        where = (
+            f"{kind} {name!r}: {field!r}" if name else f"{kind} (unnamed): {field!r}"
+        )
+        raise ValueError(f"{where} must be a list, got {type(value).__name__}")
+    return value
+
+
 # ~30K chars ≈ ~8K tokens.  Below this threshold the full plain-text
 # description fits comfortably in a single LLM context window and
 # outperforms embedding search because the LLM sees the complete
@@ -109,7 +143,7 @@ def _describe_model(model: dict, lines: list[str]) -> None:
     if data_scope:
         lines.append(f"  Data scope: {data_scope}")
 
-    cols = model.get("columns", []) or []
+    cols = _as_list(model.get("columns"), "model", name, "columns")
     described = [c for c in cols if isinstance(c, dict) and c.get("name")]
     if described:
         lines.append("  Columns:")
@@ -197,7 +231,11 @@ def _describe_cube(cube: dict, lines: list[str]) -> None:
     name = cube.get("name", "")
     base = cube.get("baseObject", "?")
     lines.append(f"### Cube: {name} (base: {base})")
-    measures = [m for m in (cube.get("measures") or []) if isinstance(m, dict)]
+    measures = [
+        m
+        for m in (_as_list(cube.get("measures"), "cube", name, "measures"))
+        if isinstance(m, dict)
+    ]
     if measures:
         lines.append("  Measures:")
         for m in measures:
@@ -210,7 +248,11 @@ def _describe_cube(cube: dict, lines: list[str]) -> None:
             if expr:
                 line += f": {expr}"
             lines.append(line)
-    dims = [d for d in (cube.get("dimensions") or []) if isinstance(d, dict)]
+    dims = [
+        d
+        for d in (_as_list(cube.get("dimensions"), "cube", name, "dimensions"))
+        if isinstance(d, dict)
+    ]
     if dims:
         lines.append("  Dimensions:")
         for d in dims:
@@ -223,7 +265,11 @@ def _describe_cube(cube: dict, lines: list[str]) -> None:
             if expr and expr != dname:
                 line += f": {expr}"
             lines.append(line)
-    tdims = [td for td in (cube.get("timeDimensions") or []) if isinstance(td, dict)]
+    tdims = [
+        td
+        for td in (_as_list(cube.get("timeDimensions"), "cube", name, "timeDimensions"))
+        if isinstance(td, dict)
+    ]
     if tdims:
         lines.append("  Time dimensions:")
         for td in tdims:
@@ -272,7 +318,7 @@ def extract_schema_items(manifest: dict) -> list[dict]:
         if not isinstance(model, dict) or not model.get("name"):
             continue
         items.append(_model_record(model, mdl_h, now))
-        for col in model.get("columns") or []:
+        for col in _as_list(model.get("columns"), "model", model["name"], "columns"):
             if not isinstance(col, dict) or not col.get("name"):
                 continue
             items.append(_column_record(col, model["name"], mdl_h, now))
@@ -290,13 +336,15 @@ def extract_schema_items(manifest: dict) -> list[dict]:
             continue
         items.append(_cube_record(cube, mdl_h, now))
         cube_name = cube.get("name", "")
-        for measure in cube.get("measures", []) or []:
+        for measure in _as_list(cube.get("measures"), "cube", cube_name, "measures"):
             if isinstance(measure, dict):
                 items.append(_measure_record(measure, cube_name, mdl_h, now))
-        for dim in cube.get("dimensions", []) or []:
+        for dim in _as_list(cube.get("dimensions"), "cube", cube_name, "dimensions"):
             if isinstance(dim, dict):
                 items.append(_cube_dimension_record(dim, cube_name, mdl_h, now))
-        for tdim in cube.get("timeDimensions", []) or []:
+        for tdim in _as_list(
+            cube.get("timeDimensions"), "cube", cube_name, "timeDimensions"
+        ):
             if isinstance(tdim, dict):
                 items.append(_time_dimension_record(tdim, cube_name, mdl_h, now))
 
@@ -309,7 +357,9 @@ def extract_schema_items(manifest: dict) -> list[dict]:
 def _model_record(model: dict, mdl_h: str, now: datetime) -> dict:
     name = model["name"]
     cols = [
-        c for c in (model.get("columns") or []) if isinstance(c, dict) and c.get("name")
+        c
+        for c in (_as_list(model.get("columns"), "model", name, "columns"))
+        if isinstance(c, dict) and c.get("name")
     ]
     col_summaries = ", ".join(f"{c['name']} ({c.get('type', '?')})" for c in cols[:20])
     pk = model.get("primaryKey") or ""
@@ -439,14 +489,18 @@ def _cube_record(cube: dict, mdl_h: str, now: datetime) -> dict:
     name = cube.get("name", "")
     base = cube.get("baseObject", "?")
     measures = ", ".join(
-        m.get("name", "") for m in (cube.get("measures") or []) if isinstance(m, dict)
+        m.get("name", "")
+        for m in (_as_list(cube.get("measures"), "cube", name, "measures"))
+        if isinstance(m, dict)
     )
     dims = ", ".join(
-        d.get("name", "") for d in (cube.get("dimensions") or []) if isinstance(d, dict)
+        d.get("name", "")
+        for d in (_as_list(cube.get("dimensions"), "cube", name, "dimensions"))
+        if isinstance(d, dict)
     )
     time_dims = ", ".join(
         td.get("name", "")
-        for td in (cube.get("timeDimensions") or [])
+        for td in (_as_list(cube.get("timeDimensions"), "cube", name, "timeDimensions"))
         if isinstance(td, dict)
     )
 

core/wren/tests/unit/test_schema_indexer_extract_nonduct.py

@@ -0,0 +1,135 @@
+"""extract_schema_items / describe_schema must skip non-dict MDL rows."""
+
+import pytest
+
+from wren.memory.schema_indexer import describe_schema, extract_schema_items
+
+
+def test_extract_skips_non_dict_models_columns_rels_views_without_raise() -> None:
+    items = extract_schema_items(
+        {
+            "models": [
+                None,
+                "bad",
+                {
+                    "name": "orders",
+                    "columns": [
+                        None,
+                        "x",
+                        {"name": "id", "type": "int"},
+                    ],
+                },
+            ],
+            "relationships": [None, {"name": "r1", "models": ["a", "b"]}],
+            "views": ["nope", {"name": "v1", "statement": "SELECT 1"}],
+        }
+    )
+    assert isinstance(items, list)
+    by_type = {i["item_type"]: i for i in items}
+    assert set(by_type) == {"model", "column", "relationship", "view"}
+    assert by_type["model"]["item_name"] == "orders"
+    assert by_type["column"]["item_name"] == "id"
+    assert by_type["column"]["model_name"] == "orders"
+    assert by_type["relationship"]["item_name"] == "r1"
+    assert by_type["view"]["item_name"] == "v1"
+    assert extract_schema_items({"models": [1, 2, 3]}) == []
+
+
+def test_extract_raises_on_non_list_nested_collections() -> None:
+    # Policy: any non-list, non-null nested collection is a structural
+    # manifest error and must raise (one rule with _iter_section / #2605's
+    # _relationship_models), not silently index an empty collection. This
+    # covers both truthy (42, 3, {"x": 1}, "nope") and falsy ({}, 0, "")
+    # non-list values — only None/missing passes through.
+    with pytest.raises(
+        ValueError, match=r"model 'm': 'columns' must be a list, got int"
+    ):
+        extract_schema_items({"models": [{"name": "m", "columns": 42}]})
+    with pytest.raises(
+        ValueError, match=r"cube 'c': 'measures' must be a list, got int"
+    ):
+        extract_schema_items({"cubes": [{"name": "c", "measures": 3}]})
+    with pytest.raises(
+        ValueError, match=r"cube 'c': 'dimensions' must be a list, got dict"
+    ):
+        extract_schema_items({"cubes": [{"name": "c", "dimensions": {"x": 1}}]})
+    with pytest.raises(
+        ValueError, match=r"cube 'c': 'timeDimensions' must be a list, got str"
+    ):
+        extract_schema_items({"cubes": [{"name": "c", "timeDimensions": "nope"}]})
+    # Falsy non-list values — the gap goldmedal flagged — also raise.
+    with pytest.raises(
+        ValueError, match=r"model 'm': 'columns' must be a list, got dict"
+    ):
+        extract_schema_items({"models": [{"name": "m", "columns": {}}]})
+    with pytest.raises(
+        ValueError, match=r"model 'm': 'columns' must be a list, got int"
+    ):
+        extract_schema_items({"models": [{"name": "m", "columns": 0}]})
+    with pytest.raises(
+        ValueError, match=r"model 'm': 'columns' must be a list, got str"
+    ):
+        extract_schema_items({"models": [{"name": "m", "columns": ""}]})
+
+
+def test_extract_unnamed_cube_falls_back_to_unnamed_form() -> None:
+    # Cubes are not required to have a name; a nameless cube with a non-list
+    # collection must not emit a bare ``cube '':``. ``measures`` is never a
+    # top-level manifest key, so we say ``cube (unnamed):`` rather than
+    # ``manifest['measures']`` — truthful about where the field lives while
+    # signalling the entity could not be identified (goldmedal's edge case).
+    with pytest.raises(
+        ValueError, match=r"cube \(unnamed\): 'measures' must be a list, got int"
+    ):
+        extract_schema_items({"cubes": [{"measures": 3}]})
+
+
+def test_extract_skips_columns_missing_name() -> None:
+    # Dict columns without a usable name are skipped, not indexed with ''.
+    items = extract_schema_items(
+        {
+            "models": [
+                {
+                    "name": "orders",
+                    "columns": [
+                        {"type": "int"},
+                        {"name": "", "type": "int"},
+                        {"name": "id", "type": "int"},
+                    ],
+                }
+            ]
+        }
+    )
+    cols = [i for i in items if i["item_type"] == "column"]
+    assert [c["item_name"] for c in cols] == ["id"]
+
+
+def test_extract_allows_none_or_empty_nested_collections() -> None:
+    # None / missing / empty-list stays a no-op (no raise): the passthrough
+    # case. Note {} / 0 / "" are NOT passthrough — see the raise test above.
+    items = extract_schema_items(
+        {"models": [{"name": "m", "columns": None}, {"name": "n"}]}
+    )
+    assert [i["item_type"] for i in items] == ["model", "model"]
+    assert extract_schema_items({"models": [{"name": "m", "columns": []}]})
+
+
+def test_top_level_non_list_section_raises() -> None:
+    with pytest.raises(ValueError, match="must be a list"):
+        extract_schema_items({"models": 5})
+    with pytest.raises(ValueError, match="must be a list"):
+        describe_schema({"models": 1})
+
+
+def test_describe_skips_non_dict_rows() -> None:
+    text = describe_schema(
+        {
+            "models": [None, {"name": "t", "columns": [{"name": "a", "type": "int"}]}],
+            "relationships": ["REL_SENTINEL_ZZZ"],
+            "views": ["VIEW_SENTINEL_ZZZ"],
+        }
+    )
+    assert "t" in text
+    # Malformed non-dict rows contribute nothing to the rendered text.
+    assert "REL_SENTINEL_ZZZ" not in text
+    assert "VIEW_SENTINEL_ZZZ" not in text