PR Review: fix(mcp): paginate taxonomy tools and log instead of swallowing errors

Executive Summary

Affected Areas: mcp_server.py (taxonomy tools: tool_list_wings, tool_list_rooms, tool_get_taxonomy)

Business Impact: Palaces with >10k drawers now return correct counts instead of silently truncated data. Errors surface in logs instead of vanishing.

Flow Changes: Three tool functions now delegate to a shared paginating generator (_iter_all_metadatas) instead of each doing a single col.get(limit=10000).

Ratings

PR Health

• Has clear description
• References ticket/issue (MCP server: mempalace_list_wings and mempalace_get_taxonomy return empty despite data in palace #171)
• Appropriate size
• Has relevant tests

High Priority Issues

(Must fix before merge)

🔗 #1: tool_status and tool_browse have the identical bug — left unfixed

Location: mempalace/mcp_server.py — tool_status (~line 50) and tool_browse (~line 121) | Confidence: ✅ HIGH

Both functions iterate col.get(include=["metadatas"])["metadatas"] with no limit or pagination, wrapped in except Exception: pass. This is the same double bug (truncation + swallowed errors) that this PR fixes for the three taxonomy tools. Leaving them unfixed means the same #171 symptom persists for two other MCP endpoints.

tool_status is especially exposed since it's the first tool agents call.

  # tool_status — current code
  try:
-     all_meta = col.get(include=["metadatas"])["metadatas"]
-     for m in all_meta:
-         w = m.get("wing", "unknown")
-         r = m.get("room", "unknown")
-         wings[w] = wings.get(w, 0) + 1
-         rooms[r] = rooms.get(r, 0) + 1
- except Exception:
-     pass
+     for m in _iter_all_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

Same pattern applies to tool_browse.

Medium Priority Issues

(Should fix, not blocking)

🚨 #2: Mid-pagination error returns partial data with no warning

Location: mempalace/mcp_server.py — _iter_all_metadatas() | Confidence: ⚠️ MED

If col.get() raises on page 2+, the generator logs the error and silently finishes. The caller receives partial counts (only page 1 data) presented as the complete result — no flag, no warning in the response dict.

For a 95k-drawer palace that fails at offset 20k, the user sees 20k drawers reported as if that's the full count.

  except Exception as e:
      logger.error("metadata iteration failed at offset %d: %s", offset, e)
+     raise  # let callers decide how to handle partial data

Alternatively, return a "warning" key in the tool response dicts so the MCP client can surface it.

🎨 #3: Tests only cover tool_list_wings — other two tools untested for pagination

Location: tests/test_mcp_server.py | Confidence: ⚠️ MED

tool_list_rooms and tool_get_taxonomy both call _iter_all_metadatas but exercise different aggregation logic (room counting, nested wing→room taxonomy). A quick parametrized or individual test for each would catch regressions in how they consume the generator — e.g., the where filter in tool_list_rooms and the setdefault nesting in tool_get_taxonomy.

def test_tool_list_rooms_paginates_with_wing_filter(monkeypatch):
    """Verify tool_list_rooms passes where-filter and paginates."""
    from unittest.mock import MagicMock
    from mempalace import mcp_server

    fake_col = MagicMock()
    fake_col.get.side_effect = [
        {"metadatas": [{"wing": "alpha", "room": "api"}] * 10000},
        {"metadatas": [{"wing": "alpha", "room": "db"}] * 3000},
    ]
    monkeypatch.setattr(mcp_server, "_get_collection", lambda create=False: fake_col)

    result = mcp_server.tool_list_rooms(wing="alpha")
    assert result["rooms"]["api"] == 10000
    assert result["rooms"]["db"] == 3000
    # Verify where-filter was forwarded
    call_kwargs = fake_col.get.call_args_list[0][1]
    assert call_kwargs["where"] == {"wing": "alpha"}

Low Priority Issues

(Nice to have)

🏗️ #4: Page size inconsistency across codebase

Location: _iter_all_metadatas vs palace_graph.py:build_graph | Confidence: ❓ LOW

_iter_all_metadatas uses a 10,000-item page size, while palace_graph.py (build_graph) uses 1,000. Both work, but a shared constant in constants.py (e.g., CHROMA_PAGE_SIZE) would prevent future drift and make the choice explicit.

🎨 #5: Naming convention — PAGE as a local variable

Location: mempalace/mcp_server.py — _iter_all_metadatas, line PAGE, offset = 10000, 0 | Confidence: ❓ LOW

PAGE reads like a constant (PEP 8 UPPER_CASE), which is fine for an effectively-constant local. But page_size would better signal that it's a batch-size parameter rather than a page index.

Flow Impact Analysis

Before (each tool independently):
  tool_list_wings ──→ col.get(limit=10000) ──→ manual loop ──→ {wings: {}}
  tool_list_rooms ──→ col.get(limit=10000) ──→ manual loop ──→ {rooms: {}}
  tool_get_taxonomy → col.get(limit=10000) ──→ manual loop ──→ {taxonomy: {}}
  (all: except Exception: pass)

After (shared generator):
  _iter_all_metadatas(col, where) ──→ paginated col.get() ──→ yields metadata dicts
       ↑                                                        ↓
  tool_list_wings ──────────────────────────────────────→ for m in generator
  tool_list_rooms ──────────────────────────────────────→ for m in generator
  tool_get_taxonomy ────────────────────────────────────→ for m in generator

  Unfixed (same old pattern):
  tool_status ──→ col.get() ──→ manual loop ──→ except Exception: pass
  tool_browse ──→ col.get() ──→ manual loop ──→ except Exception: pass

Created by Octocode MCP https://octocode.ai

Addresses review feedback in 75fd84a.

• tool_status migrated through _iter_all_metadatas, dropping its try/except: pass (review Congratulations Milla #1). Note: tool_browse referenced in review Congratulations Milla #1 does not exist in the current codebase — tool_status was the only actually-unfixed call site.
• _iter_all_metadatas now re-raises after logging (review Integrating MemPalace with SoulForge's code intelligence system #2). The existing handle_request boundary converts raised exceptions into a proper JSON-RPC error, so mid-pagination failures no longer return partial data disguised as a full result.
• New parameterized test_taxonomy_tools_paginate_beyond_10k covers tool_list_rooms, tool_get_taxonomy, and tool_status (review feat: Hermes memory provider integration #3). Existing test_tool_list_wings_logs_chromadb_error updated to assert propagation.

Deferred: mempalace/miner.py status() has the same limit=10000 truncation shape but lives in the CLI path with no shared helper access. Worth a separate PR rather than expanding this one's scope.

Full suite green.

Closing — this is superseded by recently merged PRs to develop. Thank you for the contribution!