feat(migrate): mempalace migrate-wings — normalize legacy wing names (#1675 follow-up) - #1702
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a wing-name normalization migration (migrate-wings) to strip leading and trailing separators from legacy wing names, ensuring that older palaces remain discoverable. The changes include adding a new CLI command, implementing the migration logic in mempalace/migrate.py to update drawer/closet metadata and the topics_by_wing registry, and adding corresponding tests. The review feedback highlights two potential TypeError vulnerabilities: one where metas could be None when iterating collection items, and another where tbw[new] could be None when merging topics by wing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ids = batch.ids if hasattr(batch, "ids") else batch["ids"] | ||
| metas = batch.metadatas if hasattr(batch, "metadatas") else batch["metadatas"] | ||
| if not ids: | ||
| break | ||
| for rec_id, meta in zip(ids, metas): |
There was a problem hiding this comment.
If the backend collection does not have metadata for any of the retrieved records, metas could be None. Zipping ids with None will raise a TypeError. Adding a defensive guard to default metas to a list of None values of the same length as ids prevents potential crashes.
| ids = batch.ids if hasattr(batch, "ids") else batch["ids"] | |
| metas = batch.metadatas if hasattr(batch, "metadatas") else batch["metadatas"] | |
| if not ids: | |
| break | |
| for rec_id, meta in zip(ids, metas): | |
| ids = batch.ids if hasattr(batch, "ids") else batch["ids"] | |
| metas = batch.metadatas if hasattr(batch, "metadatas") else batch["metadatas"] | |
| if not ids: | |
| break | |
| if metas is None: | |
| metas = [None] * len(ids) | |
| for rec_id, meta in zip(ids, metas): |
| if new in tbw: | ||
| merged = list(tbw[new]) | ||
| for topic in old_topics: |
There was a problem hiding this comment.
If tbw[new] is None (e.g., due to manual editing or corruption in known_entities.json), calling list(tbw[new]) will raise a TypeError. Using tbw[new] or [] provides a safe fallback to prevent potential crashes.
| if new in tbw: | |
| merged = list(tbw[new]) | |
| for topic in old_topics: | |
| if new in tbw: | |
| merged = list(tbw[new] or []) | |
| for topic in old_topics: |
There was a problem hiding this comment.
Pull request overview
Adds a new one-time mempalace migrate-wings maintenance command intended to normalize legacy wing names so pre-#1675 palaces remain discoverable after wing-name derivation changes.
Changes:
- Implement wing-name migration planner + executor that rewrites
wingmetadata in-place for drawers/closets and re-keystopics_by_wing(with collision merging). - Add
mempalace migrate-wings [--dry-run] [--yes]CLI subcommand. - Add a new test suite covering the pure planner and backend integration behavior for relabeling, dry-run, and idempotency.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
mempalace/migrate.py |
Adds the wing-name normalization migration implementation (planner, collection iteration, metadata updates, topics registry re-key). |
mempalace/cli.py |
Wires the new migrate-wings subcommand into the CLI dispatcher and argparse. |
tests/test_migrate_wings.py |
Adds planner + integration tests for the new migration behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not isinstance(wing, str) or not wing: | ||
| return None | ||
| # Apply the full normalization and explicitly strip leading/trailing | ||
| # separators. The strip is this migration's whole purpose (#1675); doing it | ||
| # here rather than relying on normalize_wing_name keeps the migration correct | ||
| # even when run against a build whose normalize_wing_name predates #1675, and | ||
| # matches the post-#1675 derivation exactly. | ||
| target = normalize_wing_name(wing).strip("_") | ||
| if not target or target == wing: | ||
| return None | ||
| return target |
| old_topics = tbw.pop(old) or [] | ||
| if new in tbw: | ||
| merged = list(tbw[new]) | ||
| for topic in old_topics: | ||
| if topic not in merged: | ||
| merged.append(topic) | ||
| tbw[new] = merged | ||
| else: | ||
| tbw[new] = old_topics |
| d_items = list(_iter_collection_items(drawers)) | ||
| all_wings = {(m or {}).get("wing") for _, m in d_items if (m or {}).get("wing")} | ||
| d_summary, d_updates = plan_wing_renames(d_items) |
| topic_renames = _plan_topics_by_wing_renames() | ||
|
|
| normalize_wing_name strips leading/trailing separators (#1675); palaces built | ||
| before that rule filed drawers under the old name (e.g. ``_alpha``). |
| # normalize_wing_name now strips leading/trailing separators, so a path-encoded | ||
| # dirname like ``-home-user-proj`` derives ``home_user_proj`` instead of | ||
| # ``_home_user_proj``. Palaces built before that rule filed drawers under the | ||
| # old, leading-underscore wing, which the new derivation no longer matches — | ||
| # searches and diary reads under the new name miss the old memories. |
Follow-up to the wing-name normalization (#1675). Palaces built before the rule filed drawers under leading/trailing-separator wing names (e.g. a Claude Code path-encoded dir `-home-user-proj` -> `_home_user_proj`); the new derivation strips those, so searches/diary reads under the new name miss the old memories — the history is split, not lost. `migrate_wing_names` (CLI: `mempalace migrate-wings [--dry-run] [--yes]`) re-keys the `wing` metadata field on drawers and closets to the normalized form, merging collisions. Design (verified against the codebase): - Drawer/closet IDs embed the wing as an opaque prefix that is never decoded back into a wing, and mining idempotency keys on `source_file`, so IDs are left untouched: closet ->drawer_id pointers stay valid and future mining still skips already-mined files. No risky extract-rebuild needed. - `topics_by_wing` registry keys are re-keyed (merging on collision). - Tunnels resolve via existing read-time normalization and need no rewrite. - Backend-agnostic (uses the get/update collection abstraction); idempotent; dry-run-able. The strip is applied by the migration itself, so it is correct regardless of whether the running build's normalize_wing_name already carries the #1675 change. Tests: pure planner coverage (strip, no-op, empty/non-string, collision) + hermetic backend integration (relabel, merge, dry-run, idempotency).
ee32e56 to
8ec438e
Compare
|
Rebased onto |
…nd surface Rebase adaptation onto current develop (348 commits, including the pluggable-backends follow-ups and wing-normalize MemPalace#1702): 1. Gate cmd_purge behind _maintenance_requires_chroma("purge"), the same guard cmd_migrate / cmd_repair / cmd_repair_status grew for pluggable backends. A palace on a non-chroma backend now gets the standard "purge is Chroma-only in this release (selected backend: ...)" message + SystemExit(2) instead of a misleading "No palace found". 2. Use MempalaceConfig().collection_name instead of hardcoding "mempalace_drawers", matching cmd_repair and the configured- collection convention develop adopted since this branch forked. The e2e purge test sets collection_name on the patched config, same shape as the existing cmd_repair tests. 76/76 in tests/test_cli.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What
Adds
mempalace migrate-wings [--dry-run] [--yes]— a one-time, idempotent migration that normalizes legacy wing names (strips leading/trailing separators) so palaces built before the wing-name rule (#1675) keep their memories discoverable under the new name.The problem: a Claude-Code path-encoded dir like
-home-user-projderived_home_user_proj. The normalized derivation strips that tohome_user_proj, so new mining/diary writes land on a different wing than the existing drawers — the history splits (it isn't lost, just stranded under the old name). This closes that split.How (design verified against the codebase)
source_file(prefetch_mined_set), not on recomputed IDs. So the migration only re-keys the metadatawingfield in place: closet→drawer_idpointers stay valid (no document rewrite) and future mining still skips already-mined files (no duplicates). This avoids a risky extract-rebuild.wing), and thetopics_by_wingregistry (re-keyed, merging on collision). Tunnels resolve via existing read-time normalization (palace_graph._normalize_wing) and need no rewrite.get/updatecollection abstraction), idempotent, dry-run-able, with collision merges reported.normalize_wing_namealready carries the fix(config): strip leading/trailing separators in normalize_wing_name #1675 change.Tests
tests/test_migrate_wings.py: pure planner coverage (strip leading/trailing, no-op on clean wings, ignore empty/non-string/all-separator, collision→same target) + hermetic backend integration (relabel, merge-into-existing, dry-run changes nothing, idempotency). 8 tests;test_migrate.py(11) andtest_cli.py(69) still pass.Merge ordering
develop: this branch is cut from the currently-reddevelop, so its CI will show the pre-existing fix: filtered search fallback + diary_write content alias #1245 failures (thesearch_memoriesC901 + 2diary_writetests) that fix(mcp): repair diary_write content alias + restore -32602 diagnostic (fixes red develop) #1700 fixes — not anything from this change. Once fix(mcp): repair diary_write content alias + restore -32602 diagnostic (fixes red develop) #1700 merges, I'll rebase and CI goes green. The migration files themselves are lint-clean and fully tested.