refactor(mcp): retire mempalace_session_recovery collection + read tool#8
Merged
refactor(mcp): retire mempalace_session_recovery collection + read tool#8
Conversation
Follow-up to fix/drop-checkpoint-write-path. With nothing writing to the recovery collection anymore (the hooks moved to verbatim-only on the parent branch), the read paths and the migration that fed the collection are dead code. Delete them. Removed in mempalace/: - palace.py: _SESSION_RECOVERY_COLLECTION constant, get_session_recovery_collection(), _CHECKPOINT_TOPICS tuple - mcp_server.py: _get_session_recovery_collection(), _recovery_collection_cache global, the topic-routing branch in tool_diary_write (now always lands in mempalace_drawers), tool_session_recovery_read() and its TOOLS dict registration - migrate.py: migrate_checkpoints_to_recovery() and the dependent _CHECKPOINT_TOPICS import - cli.py: cmd_repair --mode reorganize handler + the choice flag Removed in tests/: - tests/test_session_recovery.py — entire file (recovery-collection module test suite) - tests/test_migrate.py: TestMigrateCheckpointsToRecovery class - tests/test_mcp_server.py: TestCheckpointRouting and TestSessionRecoveryRead classes Removed in docs/: - website/reference/mcp-tools.md: mempalace_session_recovery_read section (caught by the readme-claims meta-test before deploy) Production data on disks still has the mempalace_session_recovery collection with its 763 archived entries — this PR's code change makes the collection unreachable through any MCP/CLI path, but does NOT delete the on-disk data. A separate one-shot script (scripts/phase2_purge_recovery.py per the spec) handles the collection-level delete after deploy. JP signed off on hard-delete in the spec ack. Net diff: 12 files, −1300 lines / +30 lines. 1540 tests pass, 1 skipped. Ruff lint + format clean. Stacked on #3 (which stops new writes); will auto-rebase against main when that merges. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Copilot review on #5 caught that the README still documented session_recovery_read as current, which would leave users calling a tool that returns "Unknown tool" after this PR ships. Updates the four user-facing sections that described the recovery collection as a current shipping feature: * "What just shipped" — adds 2026-05-05 update note explaining the split was retired, with the generalizable lesson preserved (a side collection without a semantic-search MCP read tool is invisible). * "Verbatim vs derivative" principle — reframes the recovery collection as the failed first attempt rather than a current example; pattern stays valid but each future sibling needs its own read surface. * "Structural retrieval fixes" bullet — describes verbatim-only end-state, links to the new spec, retains historical context for Apr 25 → May 5. * "Deterministic hook saves" bullet — updates to the new ingest-only save path, removes "recovery-collection marker" mention. * "What it looks like in production" code block — updates the systemMessage example to the new "Transcript ingest triggered (wing=...)" shape. * "P8" architectural principle — keeps it but reframes as on-hold with a precondition (read-surface parity) drawn from the cycle. * Fork-ahead tracking table — replaces the old multi-collection-split row with a verbatim-only retirement row pointing at the four PRs; also retires references to session_recovery_read in the drawer_id surfacing row; adds new row for the mining management CLI (#4). 42 README meta-tests pass (test_readme_claims.py). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Retires the now-dead mempalace_session_recovery collection surface area and associated tooling as part of the “verbatim-only” model, removing the MCP read tool, write-routing branch, and migration/CLI helpers while leaving on-disk production data untouched.
Changes:
- Removes the session-recovery collection adapter/constants and the
mempalace_session_recovery_readMCP tool + handler. - Deletes the checkpoint migration helper and the CLI “reorganize” repair mode that invoked it.
- Updates docs and prunes the associated tests that covered routing, reads, and migration.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
mempalace/mcp_server.py |
Removes recovery-collection caching/access, drops the mempalace_session_recovery_read tool, and simplifies tool_diary_write to always target the main collection. |
mempalace/palace.py |
Deletes recovery-collection constants/topics and the accessor function. |
mempalace/migrate.py |
Removes migrate_checkpoints_to_recovery and related checkpoint-topic wiring. |
mempalace/cli.py |
Removes repair --mode reorganize path and updates argparse choices/help accordingly. |
website/reference/mcp-tools.md |
Removes the mempalace_session_recovery_read documentation section. |
README.md |
Updates narrative and inventory to reflect retirement of the recovery split/tool in favor of verbatim-only. |
tests/test_session_recovery.py |
Deletes recovery-collection adapter tests. |
tests/test_migrate.py |
Deletes migration tests for checkpoint-to-recovery moves. |
tests/test_mcp_server.py |
Deletes MCP server tests for checkpoint routing and recovery reads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
845
to
850
| def cmd_repair(args): | ||
| """Rebuild palace vector index, or reorganize derivative drawers.""" | ||
| """Rebuild palace vector index.""" | ||
| import shutil | ||
| from .backends.chroma import ChromaBackend | ||
| from .migrate import ( | ||
| confirm_destructive_action, | ||
| contains_palace_database, | ||
| migrate_checkpoints_to_recovery, | ||
| ) | ||
| from .migrate import confirm_destructive_action, contains_palace_database | ||
| from .repair import TruncationDetected, check_extraction_safety |
jphein
added a commit
that referenced
this pull request
May 6, 2026
Four findings, batched into one cleanup PR. PR #6 — hooks_cli.hook_precompact docstring lie. Said "mine the transcript synchronously" but the local-mode _ingest_transcript uses subprocess.Popen (background) and daemon-strict mode is fire-and-forget HTTP. Rewrite docstring to describe the actual two-step flow: (1) transcript ingest (best-effort, may still be running when compaction starts), (2) _mine_sync of MEMPAL_DIR (real synchronous subprocess.run). PR #7 — cmd_mined unpaginated col.get + silent truncation. Old flow called col.get(where=..., include=[]) once to size the loop, then paginated with the same where clause. ChromaDB's get() has a silent ~10K id truncation, so the upfront sizing call would undercount on a wing >10K drawers. Drop the upfront sizing entirely; loop on empty-batch instead. Plus an early-exit when a batch is shorter than batch_size (saves one trailing empty get on exact-multiple wing counts). PR #8 — cmd_repair docstring stale. Said "rebuild palace vector index" but the function still dispatches max-seq-id mode. Rewrite to describe both modes + acknowledge the deleted reorganize mode. PR #10 — non-Projects dashed-project divergence. Documented limitation. Tried adding -dev-/-code-/etc. markers; reverted because those are also ambiguous (`~/dev/<project>` and `~/dev/<parent>/<project>` encode identically — a -dev-<rest> match would catch parent+project, breaking the existing test_wing_from_transcript_path_non_projects_layout test that expects the last-token fallback for `-dev-MemPalace-mempalace` → `mempalace`). Instead document the limitation in the docstring and add a regression test pinning the current behavior so future "fixes" don't break the layouts that DO want the fallback. Tests: +1 (regression test for the documented limitation). Full suite: 1552 passed, 1 skipped, ruff clean. Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
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.
Reopened after parent #3 squash-merge orphaned the original PR (#5).
Final piece of the verbatim-only retirement: with no writers and no consumers using the recovery collection, delete the read tool, the topic-routing branch, and the migration helpers. Production data on disk is untouched by this code change — a separate one-shot script handles the on-disk delete after deploy.
Original PR: #5 (closed by parent merge)
Tests: 1548 pass on rebased branch (was 1540 on the pre-rebase variant; gain comes from Phase 2a's tests being included now). Ruff clean.
🤖 Generated with Claude Code