fix(hooks): ingest only the active transcript (#2137) - #2140
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e1437e96d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "mempalace", | ||
| "mine", | ||
| str(path.parent), | ||
| str(path), |
There was a problem hiding this comment.
On the default non-daemon hook route, when two sessions targeting the same palace fire concurrently, their distinct file paths now produce different _spawn_mine PID slots, so both subprocesses launch. The per-palace lock in mine_convos is non-blocking, causing one subprocess to exit with MineAlreadyRunning; because the hook does not queue or retry that file, a final SessionEnd/PreCompact can silently leave that transcript uningested. Previously the shared parent-directory target collapsed these fires and the surviving mine scanned both files, so this change needs palace-wide serialization or a retry/queue for rejected per-file mines.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR narrows hook/daemon-triggered conversation ingestion to the active transcript file (instead of scanning the entire transcript directory) and adds support for running mempalace mine <transcript-file> --mode convos using the existing validation/scanning logic.
Changes:
- Update hook transcript ingest to pass the resolved transcript file path (including daemon dedupe key) rather than
path.parent. - Extend
scan_convos()to accept a single file path (while preserving recursive directory scanning behavior). - Add/adjust regression tests covering single-file scanning and hook/daemon payload/command targeting.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| mempalace/hooks_cli.py | Route hook/daemon transcript ingest to the resolved transcript file (and dedupe by that file). |
| mempalace/convo_miner.py | Teach scan_convos() to handle single-file sources while keeping directory recursion intact. |
| mempalace/cli.py | Update mine CLI help text to document the single-file --mode convos form. |
| tests/test_hooks_cli.py | Assert hook/daemon ingest targets the resolved transcript path (not its parent directory). |
| tests/test_convo_miner.py | Add dry-run test ensuring single-file convo mining does not scan sibling files. |
| tests/test_convo_miner_unit.py | Add unit test verifying scan_convos() returns only the requested file when given a file path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| caller can tell why an apparent conversation directory yielded no files. | ||
| """ | ||
| convo_path = Path(convo_dir).expanduser().resolve() | ||
| # A direct conversation file is a valid source. For a file, feed only | ||
| # its basename through the existing directory validation loop. | ||
| requested_path = Path(convo_dir).expanduser() |
Summary
Conflict-resolved merge of #2137 onto current
develop(after #2134 dry-run mined-state tests).mine --mode convossupport with existing validationCloses #2137.
Closes #2102.
Test plan
tests/test_convo_miner.py tests/test_convo_miner_unit.py tests/test_hooks_cli.py— 239 passed