fix: address Copilot review on PRs #6/#7/#8/#10#12
Merged
Conversation
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]>
There was a problem hiding this comment.
Pull request overview
This PR batches follow-up fixes from prior Copilot review findings across recently merged PRs, focusing on correcting/clarifying hook and CLI behavior documentation and fixing a pagination-related undercount in mempalace mined, plus pinning a known path-encoding limitation via regression test.
Changes:
- Add a regression test documenting the known limitation where non-
~/Projectsdashed project names collapse to the last token in hook-derived wings. - Fix
mempalace minedpagination to avoid relying on an unpaginatedcol.get()sizing call that can silently truncate around ~10K results. - Update docstrings for
hook_precompactandcmd_repairto reflect actual behavior/modes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/test_hooks_cli.py |
Adds a regression test documenting non-~/Projects dashed-name collapse behavior. |
mempalace/hooks_cli.py |
Updates _wing_from_transcript_path and hook_precompact docstrings to describe resolution order and flow semantics. |
mempalace/cli.py |
Fixes cmd_mined to paginate until exhaustion (no unpaginated sizing call) and updates cmd_repair docstring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+698
to
+713
| 1. ``_ingest_transcript(transcript_path)`` — local mode spawns a | ||
| background ``mempalace mine`` Popen (best-effort, non-blocking); | ||
| daemon-strict mode POSTs to ``/mine`` (the daemon serializes | ||
| under its own ``_mine_sem``, replays from a queue if a rebuild | ||
| is in progress). NOT a synchronous-write contract — by the time | ||
| compaction starts, the mine subprocess may still be running. | ||
| 2. ``_mine_sync()`` — synchronous mempalace mine of MEMPAL_DIR | ||
| (project files) when set. Blocks until exit (subprocess.run). | ||
|
|
||
| Closes Copilot finding on jphein/mempalace#6: previous docstring | ||
| claimed step 1 was synchronous; corrected to describe the actual | ||
| fire-and-forget semantics. Recoverability of "where we were" is | ||
| delivered by the verbatim transcript chunks landing in | ||
| mempalace_drawers — searching any phrase from the last few messages | ||
| finds the session. (The earlier dedicated session-recovery collection | ||
| has since been retired; see PR #8 / row 32 of CLAUDE.md.) |
Comment on lines
+814
to
+817
| # Defensive: if the backend returned fewer than batch_size, we're | ||
| # past the last page. Saves one trailing empty col.get on palaces | ||
| # whose wing-count is an exact multiple of batch_size (rare but | ||
| # cheap to handle). |
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.
Batches 4 Copilot findings across this morning's merged PRs.
_mine_syncis real synchronous run). Removes 'synchronously' claim that didn't match implementation.col.get(where=..., include=[])size-the-loop call — ChromaDB's get() silently truncates around 10K ids, so wings >10K were undercounted. Loop on empty-batch instead, plus early-exit when batch < batch_size.+1 test, 1552 passed, 1 skipped, ruff clean.
🤖 Generated with Claude Code