fix(#179): MemoryBody validate_default=True — empty wing/room now coerce - #187
Merged
Conversation
PR #186 (MemoryBody) shipped a regression: pydantic v2 skips field validators on default values, so a request to /memory omitting ``wing`` or ``room`` arrived in the handler as ``""`` instead of being coerced to the canonical defaults (``"unknown"`` / ``"discoveries"``). mempalace then rejected the empty room as "room is empty after sanitization", surfacing as HTTP 200 with ``success: false`` instead of the pre-#179 silent default behavior. The pre-#179 handler had ``body.get("wing") or "unknown"`` / ``body.get( "room") or "discoveries"`` which treated missing AND empty inputs identically — both coerced. The pydantic port only ran the validator when the field was present in the JSON. Fix: ``model_config = {"validate_default": True}`` on MemoryBody so the ``_normalize_wing`` and ``_validate_room`` validators run regardless of whether the field was provided. Caught by live-curl probe immediately after deploying #186 — the green test suite + verify-routes smoke missed it because no test exercises ``POST /memory`` directly with a missing-room body, and pydantic's own validation passes (it has a default, after all). Why this didn't affect the other write surfaces: - SilentSaveBody.wing: '' default — validator's no-op on '' matches the non-validated default; handler's warning fires either way. - MineBody.wing: 'general' default — already canonical; no-op through normalize_wing_slug. - BackfillAgeBody.wing: None default — normalize_wing_filter is pass-through on None. Only MemoryBody's two coerce-then-validate fields had the regression. 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
#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>
jphein
added a commit
that referenced
this pull request
May 29, 2026
…s incident #179 closed — wing/room canonicalization now structural across all 11 surfaces (#180→#188). Documents the #187 validate_default regression and the #185 stale-deploy incident (dead Syncthing on familiar let deploy.sh restart on stale source while reporting success). Reorders open work: #185 promoted to "actionable, unblocked" as the smallest next item. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
May 29, 2026
Converts the live-curl verification done during the #179 migration into permanent unit tests. Two contracts that had no guard: 1. Empty-wing semantics matrix — each write surface means something different by "no wing given" (/memory→"unknown", /mine→"general", /silent-save→"" , /backfill-age→None). A single side-by-side test makes a "let's unify these" refactor fail loudly instead of silently breaking callers. 2. The #187 regression — pydantic v2 skips field validators on default values unless validate_default=True. MemoryBody shipped (#186) without it; a POST omitting `room` arrived as "" and mempalace rejected it. test_memorybody_coerces_missing_* fails if that config is ever dropped. This is the test that should have existed before #186 — its absence is exactly why #187 reached production (noted in the #187 commit: "no test exercises POST /memory with a missing-room body"). Also documents why the OTHER three models don't need validate_default: their defaults ('general' / '' / None) are already canonical or pass-through, so the skipped-validator and run-validator paths agree. test_minebody_default_wing_already_canonical captures that reasoning. 21 tests, pure model construction (no HTTP/daemon/palace). Suite 535 → 556. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
May 29, 2026
…tion Closes the test gap that let #187 ship green. test_write_surface_models covers MemoryBody in isolation, but a passing model doesn't prove the ENDPOINT wires it correctly — the value dispatched to mempalace is what broke in production. These drive the full request path through main.app via TestClient, patch _call to capture the mempalace_add_drawer dispatch, and assert the canonicalized wing/room actually reach it: - missing room → dispatches "discoveries" (the exact #187 break) - missing wing → dispatches "unknown" - wing "Palace_Daemon" → dispatches "palace_daemon" - bad room → 400 AND no write dispatched (validation precedes dispatch) - empty content → still dispatches (pre-#179 permissive behavior) Verified as a genuine guard: removing validate_default=True from MemoryBody turns test_missing_room_dispatches_discoveries red (dispatches "" instead of "discoveries"), exactly reproducing #187. The model-level and HTTP-level guards both fire. This is the test the #187 commit said "should have existed before #186" — now it does, at the layer the bug actually manifested. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
May 29, 2026
The deploy smoke probed route SHAPES (does /search return "results") but never sent invalid input, so it couldn't distinguish current code from stale or broken code — happy-path responses don't change. This is the deeper half of the #185 incident: even with the new .py freshness digest, a daemon running stale bytecode (or a validator regression) would pass every existing probe. Adds probe_status() (asserts an exact HTTP status) and two NON-MUTATING behavior canaries that prove the wing/room validation layer is live in the deployed binary: - GET /search?room=<bogus> → 400 (read-side, #174) - POST /memory {room:<bogus>} → 400 (write-side, #179; rejected at MemoryBody parse time before any dispatch — non-mutating, verified by tests/test_memory_endpoint_validation::test_bad_room_rejected_400_ without_dispatch) Stale pre-#174/#179 code answers these with 200, so the canary fails the deploy instead of reporting a false "all routes verified". Scope note: these guard "validation is live" at deploy time (the stale-deploy axis). The #187-specific default-coercion case (missing room → "discoveries") is inherently mutating to probe over HTTP, so it stays guarded by the unit/HTTP test suite rather than the smoke. The two layers cover different failure axes. Verified live against familiar:8085 — both canaries return 400, full smoke green. 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.
Hotfix for #186. Pydantic v2 skips field validators on default values; without
validate_default=True, omittingwing/roomfrom a POST /memory body arrived as empty strings in the handler instead of being coerced to'unknown'/'discoveries'. Mempalace then rejected the empty room.Caught by live-curl probe of /memory on familiar immediately after deploying #186. The test suite missed it because no test exercises POST /memory with a missing-room body, and pydantic's own validation passes (it has a default, after all).
The other write surfaces (SilentSaveBody / MineBody / BackfillAgeBody) don't have the same regression because their defaults happen to be already-canonical values or pass-through (None). Only MemoryBody's two coerce-then-validate fields needed the fix.
Receipt of the bug class is in the MemoryBody docstring so a future migration won't repeat it.