fix(graph): skip malformed hallway records

CHANGELOG.md

@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ### Bug Fixes
 
+- **Malformed `hallways.json` data no longer crashes hallway operations.** The loader returned list members and truthy non-list envelope payloads without validating them, so one `null`, scalar, or partial record could break listing, deletion, recomputation, CLI, and MCP callers during attribute access or sorting. Both the current envelope and legacy bare-list formats now keep only records with a usable string ID, wing, entity pair, and safe optional count/room values; unrelated legacy fields remain intact, invalid content is reported without logging record values, and invalid UTF-8 follows the existing corrupt-file fallback. (#2270)
 - **`sweep` books a failed `stat` as a failure again.** The non-regular-file gate added in 3.7.1 reads `stat.S_ISREG(f.stat().st_mode)` inside a `try`, and its `except OSError` printed `SKIP` and moved on. A dangling symlink, a symlink loop and a file unlinked between `rglob` and the gate all raise there, and before the gate existed every one of them reached `sweep()` and was booked in `failures` — so `sweep` went from reporting a transcript it could not read to reporting success. A failed probe is now an error, not a benign file type: it is logged, printed as `WARNING`, and appended to `failures`, while a probe that succeeds and says "not regular" still skips silently. (#2221)
 - **`mempalace init` no longer tracebacks on a directory it cannot enter.** `_parse_gradle`'s `is_file()` gate sat in front of the `try` that the parser's own `except OSError` provides, so a manifest under a directory with `r` but no `x` raised `PermissionError` out of a call that used to answer "no manifest name". The gate moved inside that `try`, and `_collect_manifest_names` stats through `os.path.isfile`, which reports rather than raises. (#2221)
 - **`split` no longer blocks on a FIFO at its own output name, nor writes through a broken link.** The type gate in `main` covers the files the glob listed; `split_file` builds its output names itself, so a pre-existing named pipe at one of them wedged `write_text` in the kernel waiting for a reader. The check asks about the link itself rather than its target, because a dangling symlink at an output name reads as "nothing there" and `write_text` would create the target — landing a chunk outside the output directory. Output names that are anything but a regular file are now skipped with a `SKIP` line. (#2221)

mempalace/hallways.py

@@ -83,8 +83,52 @@ def _legacy_hallway_file() -> str:
     return os.path.join(os.path.expanduser("~"), ".mempalace", "hallways.json")
 
 
+def _is_valid_hallway_record(record) -> bool:
+    """Return whether ``record`` has the minimum traversable hallway shape.
+
+    Every generated hallway has carried these four string fields since the
+    feature shipped. Other fields are deliberately optional so pre-dynamics
+    records and hand-added metadata remain readable, but a stored count must
+    remain sortable, stored rooms must remain iterable strings, and the full
+    record must be safe for the next UTF-8 save.
+    """
+    if not isinstance(record, dict):
+        return False
+    if not all(
+        isinstance(record.get(field), str) and record[field].strip()
+        for field in ("id", "wing", "entity_a", "entity_b")
+    ):
+        return False
+
+    count = record.get("co_occurrence_count")
+    if "co_occurrence_count" in record and (
+        isinstance(count, bool) or not isinstance(count, (int, float))
+    ):
+        return False
+
+    rooms = record.get("rooms")
+    if (
+        "rooms" in record
+        and rooms is not None
+        and (not isinstance(rooms, list) or any(not isinstance(room, str) for room in rooms))
+    ):
+        return False
+
+    try:
+        json.dumps(record, ensure_ascii=False).encode("utf-8")
+    except (TypeError, ValueError, UnicodeEncodeError):
+        return False
+    return True
+
+
 def _load_hallways(config=None) -> list[dict]:
-    """Read all hallway records. Returns ``[]`` if the file is missing or corrupt.
+    """Read valid hallway records. Return ``[]`` if the file is missing or corrupt.
+
+    Both the current ``{"hallways": [...]}`` envelope and the legacy bare
+    list are accepted. Malformed records are skipped centrally so a single
+    bad value cannot crash list, delete, compute, CLI, or MCP consumers. A
+    read never rewrites the sidecar; the next successful mutation naturally
+    persists only the valid records.
 
     Backwards-compatibility: prior to this migration the hallway file was
     hardcoded at ``~/.mempalace/hallways.json`` regardless of the configured
@@ -99,14 +143,41 @@ def _load_hallways(config=None) -> list[dict]:
         try:
             with open(current_hallway_file, encoding="utf-8") as f:
                 raw = json.load(f)
-        except (OSError, json.JSONDecodeError):
+        except (OSError, UnicodeDecodeError, ValueError, RecursionError):
             logger.debug("hallways: load failed, treating as empty", exc_info=True)
             return []
         if isinstance(raw, dict) and "hallways" in raw:
-            return raw.get("hallways") or []
-        if isinstance(raw, list):
-            return raw
-        return []
+            records = raw.get("hallways")
+        elif isinstance(raw, list):
+            records = raw
+        else:
+            logger.warning(
+                "Hallways file '%s' has top-level type %s; expected a JSON list "
+                "or an object containing one; starting empty.",
+                current_hallway_file,
+                type(raw).__name__,
+            )
+            return []
+
+        if not isinstance(records, list):
+            logger.warning(
+                "Hallways file '%s' has hallway payload type %s; expected a JSON list; "
+                "starting empty.",
+                current_hallway_file,
+                type(records).__name__,
+            )
+            return []
+
+        valid_records = [record for record in records if _is_valid_hallway_record(record)]
+        skipped = len(records) - len(valid_records)
+        if skipped:
+            logger.warning(
+                "Skipped %d malformed hallway record%s from '%s'.",
+                skipped,
+                "" if skipped == 1 else "s",
+                current_hallway_file,
+            )
+        return valid_records
 
     legacy = _legacy_hallway_file()
     if legacy != current_hallway_file and os.path.exists(legacy):

tests/test_cli_hallways.py

@@ -1,5 +1,6 @@
 """Tests for the `hallways` CLI command."""
 
+import json
 from argparse import Namespace
 
 import mempalace.hallways as hallways_mod
@@ -72,3 +73,42 @@ def fake_list(wing=None, config=None):
     cmd_hallways(Namespace(wing="wing_aya", limit=50, palace=str(selected)))
 
     assert calls == [("wing_aya", str(selected))]
+
+
+def test_malformed_stored_count_is_skipped_before_cli_sort(monkeypatch, tmp_path, capsys):
+    hallway_file = tmp_path / "hallways.json"
+    monkeypatch.setattr(
+        hallways_mod,
+        "_get_hallway_file",
+        lambda *args, **kwargs: str(hallway_file),
+    )
+    monkeypatch.setattr(
+        hallways_mod,
+        "_legacy_hallway_file",
+        lambda: str(tmp_path / "legacy-hallways.json"),
+    )
+    valid = {
+        "id": "valid",
+        "wing": "wing_a",
+        "entity_a": "A",
+        "entity_b": "B",
+        "co_occurrence_count": 2,
+        "label": "A <-> B",
+    }
+    malformed = {
+        **valid,
+        "id": "malformed",
+        "co_occurrence_count": "SECRET_BAD_COUNT",
+    }
+    hallway_file.write_text(
+        json.dumps({"schema_version": 1, "hallways": [valid, malformed]}),
+        encoding="utf-8",
+    )
+
+    cmd_hallways(Namespace(wing=None, limit=50, palace=None))
+
+    captured = capsys.readouterr()
+    assert "1 hallway(s)" in captured.out
+    assert "A <-> B" in captured.out
+    assert "SECRET_BAD_COUNT" not in captured.out
+    assert "SECRET_BAD_COUNT" not in captured.err

tests/test_hallways.py

@@ -9,8 +9,12 @@
 ``mempalace/hallways.py`` and is written to make these tests pass.
 """
 
+import json
+import logging
 from unittest.mock import MagicMock, patch
 
+import pytest
+
 
 # Mock chromadb at import time so the hallways module can be loaded even
 # in environments where chromadb isn't installed. Mirrors the pattern in
@@ -54,6 +58,16 @@ def _get(limit=None, offset=0, include=None, where=None, ids=None, **kwargs):
     return col
 
 
+def _valid_hallway(hallway_id="h1", wing="wing_a", entity_a="A", entity_b="B"):
+    """Return the minimum hallway shape supported since the feature shipped."""
+    return {
+        "id": hallway_id,
+        "wing": wing,
+        "entity_a": entity_a,
+        "entity_b": entity_b,
+    }
+
+
 # ─────────────────────────────────────────────────────────────────────────────
 # Storage primitives — _load_hallways / _save_hallways
 # ─────────────────────────────────────────────────────────────────────────────
@@ -69,6 +83,20 @@ def test_load_hallways_corrupt_file_returns_empty_list(self, tmp_path, monkeypat
         hallway_file.write_text("{not valid json", encoding="utf-8")
         assert hallways_mod._load_hallways() == []
 
+    def test_load_hallways_invalid_utf8_returns_empty_list(self, tmp_path, monkeypatch):
+        hallway_file = _use_tmp_hallway_file(monkeypatch, tmp_path)
+        hallway_file.write_bytes(b'[{"id":"h1"},\xff]')
+        assert hallways_mod._load_hallways() == []
+
+    @pytest.mark.parametrize("decode_error", [ValueError("integer too large"), RecursionError()])
+    def test_load_hallways_decoder_limits_return_empty_list(
+        self, tmp_path, monkeypatch, decode_error
+    ):
+        hallway_file = _use_tmp_hallway_file(monkeypatch, tmp_path)
+        hallway_file.write_text("[]", encoding="utf-8")
+        monkeypatch.setattr(hallways_mod.json, "load", MagicMock(side_effect=decode_error))
+        assert hallways_mod._load_hallways() == []
+
     def test_save_and_load_round_trip(self, tmp_path, monkeypatch):
         _use_tmp_hallway_file(monkeypatch, tmp_path)
         sample = [
@@ -85,6 +113,114 @@ def test_save_and_load_round_trip(self, tmp_path, monkeypatch):
         hallways_mod._save_hallways(sample)
         assert hallways_mod._load_hallways() == sample
 
+    @pytest.mark.parametrize("storage_format", ["envelope", "legacy-list"])
+    def test_load_skips_malformed_records_without_logging_contents(
+        self, tmp_path, monkeypatch, caplog, storage_format
+    ):
+        hallway_file = _use_tmp_hallway_file(monkeypatch, tmp_path)
+        valid = {
+            **_valid_hallway(),
+            "strength": 0.75,
+            "legacy_optional_field": "preserved",
+        }
+        records = [
+            None,
+            "SECRET_RECORD_CONTENT",
+            42,
+            True,
+            [],
+            {},
+            {"id": "partial", "wing": "wing_a", "entity_a": "A"},
+            {"id": "wrong-type", "wing": "wing_a", "entity_a": "A", "entity_b": []},
+            {"id": "   ", "wing": "wing_a", "entity_a": "A", "entity_b": "B"},
+            {"id": "blank-wing", "wing": "\t", "entity_a": "A", "entity_b": "B"},
+            {"id": "bad-a", "wing": "wing_a", "entity_a": None, "entity_b": "B"},
+            {"id": 7, "wing": "wing_a", "entity_a": "A", "entity_b": "B"},
+            {**_valid_hallway("bad-count-1"), "co_occurrence_count": "SECRET_BAD_COUNT"},
+            {**_valid_hallway("bad-count-2"), "co_occurrence_count": None},
+            {**_valid_hallway("bad-count-3"), "co_occurrence_count": True},
+            {**_valid_hallway("bad-rooms-1"), "rooms": 7},
+            {**_valid_hallway("bad-rooms-2"), "rooms": [{}]},
+            {**_valid_hallway("bad-rooms-3"), "rooms": "room_a"},
+            {**_valid_hallway("bad-unicode"), "label": "\ud800"},
+            valid,
+        ]
+        payload = (
+            {"schema_version": 1, "hallways": records} if storage_format == "envelope" else records
+        )
+        hallway_file.write_text(json.dumps(payload), encoding="utf-8")
+
+        with caplog.at_level(logging.WARNING, logger="mempalace_hallways"):
+            loaded = hallways_mod._load_hallways()
+
+        assert loaded == [valid]
+        assert "Skipped 19 malformed hallway records" in caplog.text
+        assert str(hallway_file) in caplog.text
+        assert "SECRET_RECORD_CONTENT" not in caplog.text
+
+    @pytest.mark.parametrize(
+        ("payload", "private_marker"),
+        [
+            ("SECRET_ROOT_CONTENT", "SECRET_ROOT_CONTENT"),
+            (
+                {"schema_version": 1, "hallways": {"SECRET_FIELD_KEY": "private"}},
+                "SECRET_FIELD_KEY",
+            ),
+        ],
+    )
+    def test_load_rejects_non_list_payload_without_logging_contents(
+        self, tmp_path, monkeypatch, caplog, payload, private_marker
+    ):
+        hallway_file = _use_tmp_hallway_file(monkeypatch, tmp_path)
+        hallway_file.write_text(json.dumps(payload), encoding="utf-8")
+
+        with caplog.at_level(logging.WARNING, logger="mempalace_hallways"):
+            loaded = hallways_mod._load_hallways()
+
+        assert loaded == []
+        assert "expected a JSON list" in caplog.text
+        assert str(hallway_file) in caplog.text
+        assert private_marker not in caplog.text
+
+    @pytest.mark.parametrize("storage_format", ["envelope", "legacy-list"])
+    def test_list_and_delete_recover_from_mixed_records(
+        self, tmp_path, monkeypatch, storage_format
+    ):
+        hallway_file = _use_tmp_hallway_file(monkeypatch, tmp_path)
+        valid = _valid_hallway()
+        records = [
+            None,
+            "junk",
+            {**_valid_hallway("bad-count"), "co_occurrence_count": "SECRET_BAD_COUNT"},
+            {**_valid_hallway("bad-unicode"), "label": "\ud800"},
+            valid,
+        ]
+        payload = (
+            {"schema_version": 1, "hallways": records} if storage_format == "envelope" else records
+        )
+        original = json.dumps(payload)
+        hallway_file.write_text(original, encoding="utf-8")
+
+        assert hallways_mod.list_hallways(wing="wing_a") == [valid]
+        assert hallway_file.read_text(encoding="utf-8") == original
+
+        assert hallways_mod.delete_hallway("h1") is True
+        assert json.loads(hallway_file.read_text(encoding="utf-8")) == {
+            "schema_version": 1,
+            "hallways": [],
+        }
+
+    def test_unknown_delete_preserves_mixed_file_bytes(self, tmp_path, monkeypatch):
+        hallway_file = _use_tmp_hallway_file(monkeypatch, tmp_path)
+        original = json.dumps(
+            {"schema_version": 1, "hallways": [None, _valid_hallway()]},
+            indent=3,
+        )
+        hallway_file.write_text(original, encoding="utf-8")
+
+        assert hallways_mod.delete_hallway("missing") is False
+        assert hallway_file.read_text(encoding="utf-8") == original
+
 
 # ─────────────────────────────────────────────────────────────────────────────
 # compute_hallways_for_wing — entity-pair co-occurrence algorithm
@@ -110,6 +246,32 @@ def test_explicit_config_scopes_persistence_to_selected_palace(self, tmp_path):
         assert hallways_mod.list_hallways(config=selected_cfg) == created
         assert hallways_mod.list_hallways(config=default_cfg) == []
 
+    def test_compute_skips_malformed_records_and_preserves_valid_other_wings(
+        self, tmp_path, monkeypatch
+    ):
+        hallway_file = _use_tmp_hallway_file(monkeypatch, tmp_path)
+        other_wing = _valid_hallway("other-hallway", "wing_b", "B", "C")
+        hallway_file.write_text(
+            json.dumps({"schema_version": 1, "hallways": [None, "junk", other_wing]}),
+            encoding="utf-8",
+        )
+        col = _fake_collection(
+            [
+                {"wing": "wing_a", "room": "room_1", "entities": "A;B"},
+                {"wing": "wing_a", "room": "room_2", "entities": "A;B"},
+            ]
+        )
+
+        created = hallways_mod.compute_hallways_for_wing("wing_a", col=col)
+
+        assert len(created) == 1
+        persisted = json.loads(hallway_file.read_text(encoding="utf-8"))
+        assert persisted["schema_version"] == 1
+        assert {record["id"] for record in persisted["hallways"]} == {
+            "other-hallway",
+            created[0]["id"],
+        }
+
     def test_returns_empty_for_unknown_wing(self, tmp_path, monkeypatch):
         """Wing with no drawers → no hallways, no crash."""
         _use_tmp_hallway_file(monkeypatch, tmp_path)
@@ -315,15 +477,15 @@ def test_delete_hallway_removes_record(self, tmp_path, monkeypatch):
 
     def test_delete_hallway_unknown_id_returns_false(self, tmp_path, monkeypatch):
         _use_tmp_hallway_file(monkeypatch, tmp_path)
-        hallways_mod._save_hallways([{"id": "h1", "wing": "wing_aya"}])
+        hallways_mod._save_hallways([_valid_hallway("h1", "wing_aya", "Aya", "Lumi")])
         assert hallways_mod.delete_hallway("nonexistent") is False
 
     def test_delete_hallway_uses_selected_palace_config(self, tmp_path):
         from mempalace.config import MempalaceConfig
 
         default_cfg = MempalaceConfig(palace_path=tmp_path / "default" / "palace")
         selected_cfg = MempalaceConfig(palace_path=tmp_path / "selected" / "palace")
-        record = {"id": "h1", "wing": "wing_aya"}
+        record = _valid_hallway("h1", "wing_aya", "Aya", "Lumi")
         hallways_mod._save_hallways([record], default_cfg)
         hallways_mod._save_hallways([record], selected_cfg)