refactor(mcp): retire mempalace_session_recovery collection + read tool#5
Closed
jphein wants to merge 6 commits intofix/drop-checkpoint-write-pathfrom
Closed
refactor(mcp): retire mempalace_session_recovery collection + read tool#5jphein wants to merge 6 commits intofix/drop-checkpoint-write-pathfrom
jphein wants to merge 6 commits intofix/drop-checkpoint-write-pathfrom
Conversation
Daemon-strict mode (introduced 2026-04-24 in commits 8c90c0f + 0e97b19 to fix the HNSW drift incident) skipped all three local mining paths when PALACE_DAEMON_URL was set, on the assumption a daemon-side writer would do the work instead. The diary checkpoint half got that writer via /silent-save, but the transcript-ingest half did not — so for ~11 days every Claude Code session left a checkpoint summary in the recovery collection and zero verbatim transcript drawers in mempalace_drawers. mempalace_search lost visibility into recent sessions even though MCP, daemon, and HNSW were all healthy. Replace the three skip-and-bail branches with POSTs to the daemon's existing /mine endpoint via a new _post_daemon_mine() helper: * _maybe_auto_ingest (background project mine on Stop) * _mine_sync (synchronous project mine on PreCompact) * _ingest_transcript (transcript convo mine on Stop / PreCompact) The daemon already has /mine; this PR just wires the hook to call it. Path translation (so the daemon can find client-side paths in its own filesystem) is handled daemon-side via PALACE_DAEMON_PATH_MAP — see the companion palace-daemon PR. Behavior change: transcript ingest now routes to the project wing derived via _wing_from_transcript_path(), matching the diary checkpoint behavior at hook_stop:740. Previously hardcoded "sessions"; now produces e.g. "wing_memorypalace" / "wing_realmwatch" per transcript. The "sessions" wing remains as the fallback for paths that don't match the Claude Code project layout (now spelled "wing_sessions" by the existing helper). Tests: * 5 existing tests now use clear=True on patch.dict so PALACE_DAEMON_URL in the developer env stops leaking into local-spawn assertions. * 6 new tests cover _post_daemon_mine (URL/body/api-key/error-paths) and the daemon-routed branches in all three mining functions. * Full suite green: 1552 passed, 1 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three findings from Copilot's review pass plus one CI lint fix: 1. **Wing derivation parity** (Copilot L330, L354). _maybe_auto_ingest and _mine_sync hardcoded wing="general" in the daemon-routed branch, but the local-spawn fallback omits --wing so convo_miner derives from Path(mine_dir).name. The two paths produced different wings for identical input. Add _wing_from_mine_dir() that mirrors normalize_wing_name(Path(mine_dir).name) and use it in both daemon-routed branches. 2. **Reuse transcript validator** (Copilot L597). _ingest_transcript bypassed _validate_transcript_path, accepting any harness-supplied .jsonl path including ".." traversal sequences. _count_human_messages already validates; mirror that here so the same traversal/extension guards apply on the ingest path. 3. **Timeout adequacy**. Daemon /mine awaits proc.communicate() (blocks until subprocess finishes). 10s was too short for typical mines; bump to 30s. The hook-side timeout is cosmetic — even on timeout the daemon-side mine completes since the subprocess is independent of the HTTP connection — but a misleading log entry still reads like a failure. 4. **Ruff format**. CI's `ruff format --check .` flagged blank-line gaps inside the new test class _FakeResp. Auto-formatted. New tests: - test_ingest_transcript_rejects_traversal — closes the security finding - test_ingest_transcript_rejects_wrong_extension — same guard family - test_wing_from_mine_dir_normalizes — covers the new helper - test_maybe_auto_ingest_routes_through_daemon updated to assert wing derivation (project dirname "my-project" → "my_project") - test_mine_sync_routes_through_daemon updated likewise 82 passed, 1 skipped. Ruff format + lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Stop hook used to do two things on each fire:
(a) write a 1-KB checkpoint summary diary entry into the
mempalace_session_recovery collection
(b) auto-mine the verbatim transcript into mempalace_drawers
The summary in (a) is redundant once (b) is searchable, AND the
recovery collection had no semantic-search MCP surface — so
checkpoints were structurally invisible to mempalace_search. Net
effect from a user's seat: agents (and JP) couldn't find recent
session content via search even though everything was on disk.
Drop the summary half. Verbatim transcript chunks already contain
every word a checkpoint summary would have surfaced. Leaving only
the transcript-ingest path matches the verbatim-only architecture
described in docs/superpowers/specs/2026-05-05-verbatim-only-design.md.
Changes:
* hook_stop silent path: remove _save_diary_direct call. Save marker
advances unconditionally (fire-and-forget mining doesn't have a
"confirmed save" signal to gate on; failure detection moves to
daemon-side observability — hook.log + systemd journal).
* hook_precompact: remove the recovery-marker write. Mine + compaction
proceed as before.
* systemMessage shape changes from "✦ N memories woven into the
palace — themes" to "✦ Transcript ingest triggered (wing=...)".
Lighter, accurate, no false count.
* Delete the now-unused _save_diary_direct function (~120 LOC) and
its dependencies _extract_themes + _THEME_STOPWORDS (~30 LOC).
* Update 4 hook tests + 1 OSError test to mock _ingest_transcript
rather than _save_diary_direct, and to expect the new
systemMessage shape.
Scope deliberately limited: this PR removes hook-side WRITES.
mempalace_session_recovery collection still exists; the
mempalace_session_recovery_read MCP tool still works against the
existing 763 archived entries; mempalace.migrate is untouched.
A follow-up PR will dispose of the collection itself once this
change has run in production for a verification window.
Net diff: −230 / +53 LOC. 1558 tests pass, 1 skipped, lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the architectural shift driving this PR family — drop checkpoint summaries, expose only the verbatim transcripts that mempalace_search can already reach. Documents: * Why: the recovery collection had no semantic-search MCP surface, so checkpoints in it were structurally invisible to search * What: hook write-path deletion (this PR) + collection disposal (follow-up PR) * How: code-deletion checklist for both repos, migration plan with tarball safety net, test plan * Open questions queued for review at merge time Spec gates the destructive parts on the answers to those open questions; the write-path removal in this PR is scoped narrow enough to land without them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Removes the now-dead session-recovery collection integration that became obsolete after the earlier hook write-path change, simplifying MemPalace's MCP, migration, CLI, and test surfaces around the verbatim-only model.
Changes:
- Deletes the retired session-recovery collection helpers, migration path, and MCP read tool.
- Simplifies diary writes so all entries go to the main
mempalace_drawerscollection. - Removes the associated CLI mode, tests, and reference docs for the deleted feature.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
website/reference/mcp-tools.md |
Removes the reference entry for the retired MCP recovery-read tool. |
tests/test_session_recovery.py |
Deletes tests for the removed recovery collection adapter. |
tests/test_migrate.py |
Deletes migration tests for moving checkpoints into recovery storage. |
tests/test_mcp_server.py |
Deletes MCP tests for checkpoint routing and recovery reads. |
mempalace/palace.py |
Removes recovery-collection constants and helper accessor. |
mempalace/migrate.py |
Removes the checkpoint-to-recovery migration implementation. |
mempalace/mcp_server.py |
Removes recovery collection caching, tool handler, tool registration, and write routing. |
mempalace/cli.py |
Removes repair --mode reorganize and related CLI help text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -347,23 +347,6 @@ Read recent diary entries. | |||
|
|
|||
| --- | |||
|
|
|||
jphein
added a commit
that referenced
this pull request
May 6, 2026
Copilot review on #3 caught that the docstring claimed the recovery collection was "now-deleted," but this PR only removes the write call site — collection retirement is the follow-up PR #5. Reword to match actual scope: the recovery-marker write is removed here; the collection still exists and gets disposed in #5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
5 tasks
Owner
Author
|
Copilot review addressed in commit
The fork-ahead tracking table now points at this PR family rather than the retired multi-collection-split entry, and adds a row for #4 ( Stacked on #3 — 42 README meta-tests pass; ruff clean. |
jphein
added a commit
that referenced
this pull request
May 6, 2026
Copilot review on #3 caught that the docstring claimed the recovery collection was "now-deleted," but this PR only removes the write call site — collection retirement is the follow-up PR #5. Reword to match actual scope: the recovery-marker write is removed here; the collection still exists and gets disposed in #5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
75069ef to
d396b3c
Compare
jphein
added a commit
that referenced
this pull request
May 6, 2026
* fix(hooks): drop checkpoint write path from Stop + PreCompact
The Stop hook used to do two things on each fire:
(a) write a 1-KB checkpoint summary diary entry into the
mempalace_session_recovery collection
(b) auto-mine the verbatim transcript into mempalace_drawers
The summary in (a) is redundant once (b) is searchable, AND the
recovery collection had no semantic-search MCP surface — so
checkpoints were structurally invisible to mempalace_search. Net
effect from a user's seat: agents (and JP) couldn't find recent
session content via search even though everything was on disk.
Drop the summary half. Verbatim transcript chunks already contain
every word a checkpoint summary would have surfaced. Leaving only
the transcript-ingest path matches the verbatim-only architecture
described in docs/superpowers/specs/2026-05-05-verbatim-only-design.md.
Changes:
* hook_stop silent path: remove _save_diary_direct call. Save marker
advances unconditionally (fire-and-forget mining doesn't have a
"confirmed save" signal to gate on; failure detection moves to
daemon-side observability — hook.log + systemd journal).
* hook_precompact: remove the recovery-marker write. Mine + compaction
proceed as before.
* systemMessage shape changes from "✦ N memories woven into the
palace — themes" to "✦ Transcript ingest triggered (wing=...)".
Lighter, accurate, no false count.
* Delete the now-unused _save_diary_direct function (~120 LOC) and
its dependencies _extract_themes + _THEME_STOPWORDS (~30 LOC).
* Update 4 hook tests + 1 OSError test to mock _ingest_transcript
rather than _save_diary_direct, and to expect the new
systemMessage shape.
Scope deliberately limited: this PR removes hook-side WRITES.
mempalace_session_recovery collection still exists; the
mempalace_session_recovery_read MCP tool still works against the
existing 763 archived entries; mempalace.migrate is untouched.
A follow-up PR will dispose of the collection itself once this
change has run in production for a verification window.
Net diff: −230 / +53 LOC. 1558 tests pass, 1 skipped, lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(spec): verbatim-only architecture design
Captures the architectural shift driving this PR family — drop
checkpoint summaries, expose only the verbatim transcripts that
mempalace_search can already reach. Documents:
* Why: the recovery collection had no semantic-search MCP surface,
so checkpoints in it were structurally invisible to search
* What: hook write-path deletion (this PR) + collection disposal
(follow-up PR)
* How: code-deletion checklist for both repos, migration plan with
tarball safety net, test plan
* Open questions queued for review at merge time
Spec gates the destructive parts on the answers to those open
questions; the write-path removal in this PR is scoped narrow
enough to land without them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(hooks): clarify hook_precompact docstring scope
Copilot review on #3 caught that the docstring claimed
the recovery collection was "now-deleted," but this PR only removes
the write call site — collection retirement is the follow-up PR #5.
Reword to match actual scope: the recovery-marker write is removed
here; the collection still exists and gets disposed in #5.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
May 6, 2026
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) <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
May 6, 2026
…ol (#8) * refactor(mcp): retire mempalace_session_recovery collection + read tool 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) <noreply@anthropic.com> * docs(README): align with verbatim-only retirement 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) <noreply@anthropic.com> --------- 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.
Summary
Follow-up to #3 (`fix/drop-checkpoint-write-path`). With the parent branch stopping all writes to `mempalace_session_recovery`, the read paths and migration code that fed it become dead. This PR deletes them.
Stacked on #3
Base branch is `fix/drop-checkpoint-write-path`. When that merges, this PR auto-rebases against `main` and the diff narrows to just this commit. Reviewers see only the new deletions.
Why
The recovery collection existed to keep checkpoint summaries from polluting `mempalace_search` results. PR #3 stopped writing to it. With nothing writing, the read tool surfaces only frozen historical data, the migration code has no work to do, and the topic-routing branch is dead. Keeping any of it would invite future-self to wire it back up. Cleaner to delete.
Production data is NOT touched
This PR makes the recovery collection unreachable through any MCP / CLI / hook path. It does NOT delete the 763 archived entries on disk in `mempalace_session_recovery`. A separate one-shot script (`scripts/phase2_purge_recovery.py` per the spec at `docs/superpowers/specs/2026-05-05-verbatim-only-design.md`) handles the collection-level delete after deploy.
That script is intentionally NOT part of this PR. Reasons:
JP signed off on hard-delete with tarball backup in the spec.
Risk
Test plan
🤖 Generated with Claude Code