fix: normalize wing on read endpoints (symmetric write/read contract) - #175
Merged
Conversation
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) <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
jphein
added a commit
that referenced
this pull request
May 29, 2026
This session found two bugs of the same shape — write canonicalizes the value, read doesn't — at the room (#174) and wing (#175) layers. The lesson generalizes: any time a write normalizes/validates input into a canonical form, every read surface must apply the same normalization/validation, or you get silent empty-result bugs where the same data is unreachable by a slightly-different query. Codify the discovery pattern so future code review catches it proactively rather than after-the-fact: "when you find a write that canonicalizes a value, grep for every endpoint that reads that value and verify they share the canonicalization." Sits next to the silent-exception convention (#169) and the library- version awareness section as the third recurring pattern this session's autonomous loop converged on. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
May 29, 2026
Third instance of the write/read symmetry pattern (#174 room PATCH, #175 wing reads, this PR). /silent-save passed wing through to tool_diary_write unchanged, while /memory normalizes via _normalize_wing_slug and all read endpoints normalize queries via normalize_wing_filter (post #175). Hook with wing="Palace_Daemon" → /silent-save stores diary entry under "Palace_Daemon" (literal) → /search?wing=Palace_Daemon normalizes to "palace_daemon" → MISS Hook with wing="palace_daemon" already lowercased → /silent-save stores under "palace_daemon" (works) → /search?wing=Palace_Daemon normalizes to "palace_daemon" → HIT Same data integrity hole as the asymmetric room PATCH from #174: the same conceptual value reached different stored values depending on which write endpoint you used. Fix: apply _normalize_wing_slug in _do_silent_save_write before forwarding to tool_diary_write. Empty wing stays empty (the /silent-save handler already warns on empty rather than coercing to "unknown", so we preserve that contract). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
May 29, 2026
Continuation of the wing-canonicalization sweep started in #175 and #177. Three more sites pass wing to subprocess without normalizing: 1. POST /mine handler — writes drawers via `mempalace mine --wing ...` subprocess. Pre-fix: /mine with wing="Palace_Daemon" stored drawers under that literal; /search?wing=Palace_Daemon normalizes to "palace_daemon" (post-#175) → miss. 2. POST /backfill-age handler — uses wing as a *filter* (restrict backfill to one wing). Pre-fix: backfill-age with wing="Palace_Daemon" filtered for that literal; drawers stored as "palace_daemon" weren't touched. 3. WatcherService._internal_mine — same shape as /mine. Reads wing from PALACE_WATCH_DIRS env which may contain mixed-case entries. All three now route through _normalize_wing_slug (for write paths) or _rooms.normalize_wing_filter (for the /backfill-age filter path). All the conceptual variants of "the same wing" now resolve to the same canonical slug regardless of which endpoint or env var supplied them. This closes the wing-canonicalization asymmetry sweep: /memory, /silent-save, /mine, /backfill-age, watcher auto-mine, and the six read endpoints (/search, /search/hybrid, /search/keyword, /search/age-fused, /search/fast, /list) all share the same normalization contract. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
May 29, 2026
…ion (#180) Implements #179's Option A for the three query-param read endpoints. After PRs #175/#177/#178 sprinkled normalize/validate calls at 11 sites, this PR moves enforcement up to the request-parse layer for the simplest subset (query-param endpoints) so future additions get the contract structurally rather than via per-site convention. Adds two FastAPI dependency factories in rooms.py: - rooms.wing_filter_dep — wraps normalize_wing_filter - rooms.room_validator_dep — wraps validate_room_or_raise Migrates the three query-param read endpoints: GET /search (wing + room) GET /list (wing + room) GET /search/fast (wing only) Each endpoint declares: wing: str | None = Depends(_rooms.wing_filter_dep) room: str | None = Depends(_rooms.room_validator_dep) and the handler body just uses the canonicalized values. The previous inline canonicalize/validate calls are removed (they would have been redundant — the dependency runs before the handler). POST endpoints (/search/hybrid, /search/keyword, /search/age-fused) stay on inline calls — they parse JSON bodies and would need pydantic models to use Depends. Filed as a follow-up in #179. Reordered the `import rooms as _rooms` to top-of-file so the Depends() references in route signatures resolve at module-load time (was previously imported in the #101 twelfth-slice block much later in the file). Tests: 531 → 534 (3 new dep tests in test_room_validation.py). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
May 29, 2026
#188) Fifth and final write surface for #179. The watcher isn't an HTTP endpoint so it can't take a pydantic body — but the same architectural goal applies: canonicalize wing at the *input* boundary so all downstream code can trust the WatchTarget is canonical. Pre-#179: parse_watch_dirs ran ``normalize_wing_name`` only on path-derived wings (when env entry was bare ``path`` with no ``=wing``). Explicit ``path=Wing_Name`` env entries were passed through verbatim, and a defensive ``_normalize_wing_slug`` ran later in _internal_mine before subprocess spawn. Post-#179: parse_watch_dirs normalizes both branches (path-derived AND explicit) through normalize_wing_name. WatchTargets always carry canonical slugs. The use-time normalize in _internal_mine drops to a comment explaining the new invariant. Test added: ``Palace_Daemon`` and ``palace_daemon`` env entries produce identical WatchTarget.wing values ("palace_daemon"). Was the shape of the pre-#175/#178 bug class — mixed-case writes paired with canonical reads producing empty result sets. This closes the wing-canonicalization migration that began with #172. All 11 wing/room-accepting sites in palace-daemon now canonicalize at the input boundary: Read side (#180): GET /search, GET /list, GET /search/fast — FastAPI Depends() Write body (#181, #182, #183, #184, #186, #187): POST /search/keyword, POST /search/hybrid, POST /search/age-fused, POST /backfill-age, POST /silent-save, POST /mine, POST /memory — pydantic body models in search_models.py Internal env (this PR): PALACE_WATCH_DIRS via watcher.parse_watch_dirs Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
POST /memorynormalizes wing slug on write (Palace_Daemon→palace_daemon) but read endpoints passed the caller's wing through unchanged. Live repro:Same shape as the PATCH /memory{room} asymmetric contract bug from #174 — write and read disagreed on what counted as 'the same value'.
Fix
Add
rooms.normalize_wing_filter(s)— read-side wrapper that:normalize_wing_slugto match write-side normalizationApplied at all six read endpoints:
/search,/search/hybrid,/search/keyword,/search/age-fused,/search/fast,/list.Tests
+7 new tests in
test_room_validation.py::TestNormalizeWingFiltercovering None, empty, mixed-case, wing_ prefix, idempotence, whitespace-only, and garbage-input edge cases.Total: 524 → 531 tests passing.