fix/mcp-taxonomy-pagination (#171)

[Formatted]

## What does this PR do?

Fixes a silent data-loss bug (#171) in the four MCP taxonomy tools (mempalace_status, mempalace_list_wings, mempalace_list_rooms, mempalace_get_taxonomy). On palaces with more than ~10k drawers, all four functions called col.get(limit=10000) in a single shot and wrapped the whole thing in except Exception: pass. Two things could go wrong:

1. ChromaDB silently caps or returns None for metadatas on large collections, causing a TypeError: 'NoneType' is not iterable that gets swallowed — result: {"wings": {}} on a 96k-drawer palace.
2. Even when it doesn't raise, a single 10k-item fetch misses everything beyond the cap.

The fix extracts a shared _iter_metadatas(col, where=None) generator that pages through the collection in batches of 500 (the same pattern already used correctly in layers.py), guards against None metadatas at each page, and logs a warning instead of silently swallowing errors. All four taxonomy functions are updated to use it.

## How to test

# Install deps
pip install -e ".[dev]"

# Run the new pagination tests (no network needed — uses in-memory fixtures)
python -m pytest tests/test_mcp_server.py::TestTaxonomyPagination -v

# Full suite
python -m pytest tests/ -v

The three new tests in TestTaxonomyPagination monkey-patch _TAXONOMY_BATCH = 2 to force multi-page fetching on a small fixture, then assert all items are counted across pages. A third test confirms graceful handling when ChromaDB returns None for metadatas.

## Checklist

• Tests pass (python -m pytest tests/ -v)
• No hardcoded paths
• Linter passes (ruff check .)

[bgauryy]

PR Review: fix/mcp-taxonomy-pagination (#171)

Executive Summary

Aspect	Value
PR Goal	Fix silent data-loss in 4 MCP taxonomy tools by paginating ChromaDB metadata fetches
Files Changed	2 (mempalace/mcp_server.py, tests/test_mcp_server.py)
Risk Level	🟢 LOW — Well-scoped bug fix replacing 4 copy-pasted col.get(limit=10000) patterns with a shared paginated generator
Review Effort	2/5 — Small, focused change with clear intent
Recommendation	✅ APPROVE (with minor suggestions)

Affected Areas: tool_status, tool_list_wings, tool_list_rooms, tool_get_taxonomy in mcp_server.py

Business Impact: Palaces with >10k drawers now return complete taxonomy data instead of silently truncated or empty results. This directly fixes incorrect mempalace_status output that AI assistants rely on for palace navigation.

Flow Changes: All 4 taxonomy tools now iterate metadata in 500-item pages via _iter_metadatas() instead of a single col.get(limit=10000). Error handling moves from bare except Exception: pass (returns empty) to per-page except with logger.warning (returns partial data collected so far).

Ratings

Aspect	Score
Correctness	4/5
Security	5/5
Performance	5/5
Maintainability	5/5

PR Health

• Has clear description (references issue MCP server: mempalace_list_wings and mempalace_get_taxonomy return empty despite data in palace #171, explains root cause and fix)
• References ticket/issue (MCP server: mempalace_list_wings and mempalace_get_taxonomy return empty despite data in palace #171)
• Appropriate size (131 additions, 36 deletions across 2 files)
• Has relevant tests (3 new tests covering pagination and None-metadatas resilience)

---

Medium Priority Issues

(Should fix, not blocking)

🐛 #1: tool_status can return inconsistent total_drawers vs wing/room sums on mid-pagination failure

Location: mempalace/mcp_server.py — tool_status + _iter_metadatas | Confidence: ⚠️ MED

tool_status gets the authoritative count from col.count() but builds wing/room breakdowns from _iter_metadatas(). If ChromaDB raises on page 3 of 10, the generator silently stops and the tool returns total_drawers: 5000 alongside wing counts that sum to ~1200. An AI assistant consuming this output would see contradictory numbers with no signal that data is partial.

The old code had the same risk class (empty on failure) but could not return partial data — it was all-or-nothing. Now partial is possible, which is arguably more misleading.

Consider either:

• (a) Summing yielded metadata and comparing to col.count(), adding a "partial": true flag if they diverge, or
• (b) Accepting this as an improvement over the old behavior (which is a valid position — logging the warning is already better than pass).

+    total_from_meta = 0
     for m in _iter_metadatas(col):
         w = m.get("wing", "unknown")
         r = m.get("room", "unknown")
         wings[w] = wings.get(w, 0) + 1
         rooms[r] = rooms.get(r, 0) + 1
+        total_from_meta += 1
     return {
         "total_drawers": count,
         "wings": wings,
         "rooms": rooms,
+        "partial": total_from_meta < count,
         "palace_path": _config.palace_path,
         ...
     }

---

🎨 #2: if where: uses truthiness instead of is not None

Location: mempalace/mcp_server.py:79 — _iter_metadatas | Confidence: ⚠️ MED

if where:
    kwargs["where"] = where

This works for current callers (which pass None or a populated dict), but if where: evaluates to False for an empty dict {}. If a future caller passes where={}, it would be silently ignored — ChromaDB would return unfiltered results instead of raising on the invalid filter.

Using is not None is more precise and defensive:

-        if where:
+        if where is not None:
             kwargs["where"] = where

---

Low Priority Issues

(Nice to have)

🔗 #3: No pagination test for tool_list_rooms with where filter

Location: tests/test_mcp_server.py — TestTaxonomyPagination | Confidence: ✅ HIGH

The PR tests pagination for tool_list_wings and tool_get_taxonomy, but tool_list_rooms(wing=...) is the only caller that passes the where parameter to _iter_metadatas. The where path is not exercised under pagination conditions (_TAXONOMY_BATCH=2). Existing test_list_rooms_filtered in TestReadTools covers it without forced pagination.

Adding a pagination-specific variant would close this gap:

def test_list_rooms_filtered_counts_all_pages(
    self, monkeypatch, config, palace_path, seeded_collection, kg
):
    import mempalace.mcp_server as mcp_module

    _patch_mcp_server(monkeypatch, config, palace_path, kg)
    monkeypatch.setattr(mcp_module, "_TAXONOMY_BATCH", 2)

    from mempalace.mcp_server import tool_list_rooms

    result = tool_list_rooms(wing="project")
    assert result["rooms"]["backend"] == 2
    assert result["rooms"]["frontend"] == 1
    assert "planning" not in result["rooms"]

---

🚨 #4: _iter_metadatas generator hides errors from callers

Location: mempalace/mcp_server.py:82-86 — _iter_metadatas exception handler | Confidence: ⚠️ MED

When ChromaDB raises mid-iteration, the generator logs a warning and returns. Callers have no way to distinguish "all data fetched successfully" from "stopped early due to error". While the logger.warning is better than the old except Exception: pass, callers that need to know about incomplete data (like tool_status in issue #1 above) cannot detect it.

This is a design trade-off, not necessarily a defect. If partial data is acceptable for taxonomy browsing, this is fine. If callers ever need to signal data completeness, the generator interface would need to change (e.g., raise after yielding, or return a sentinel).

No code change suggested — just flagging the design decision.

---

Summary

This is a clean, well-scoped bug fix. The _iter_metadatas generator is a solid DRY improvement over 4 identical copy-pasted patterns. ChromaDB's offset/limit pagination is verified as stable API. The 500-item batch size balances memory efficiency with round-trip count. Tests are smart (forcing _TAXONOMY_BATCH=2 to exercise pagination with just 4 items) and the None-metadatas edge case test is valuable.

The only material concern is that tool_status can now return self-contradictory data on partial failure (issue #1), which the old code could not. This is arguably still better than the old behavior of returning empty data, but worth considering a "partial" flag for consumers.

---

Created by Octocode MCP https://octocode.ai

[milla-jovovich]

Hey — I've taken a look and ran it through CLI. Thanks for the fix on the MCP taxonomy pagination bug! There are two open PRs addressing #171 and going with #307 because its title framing ("log instead of swallowing errors") is the correct fix shape — silent errors in MCP tools are user-hostile. Closing this one as superseded. Really appreciate the contribution. 💜

[bensig]

Reopening — this was closed too quickly and your work deserves proper review. Standing by for merge once we clear the current security baseline. Thank you for the contribution.

[Formatted]

#307 is the cleaner solution, closing this one