From ecd44f7cb7fa544adad60f3149c6a66df2461611 Mon Sep 17 00:00:00 2001 From: Mikhail Valentsev Date: Wed, 15 Apr 2026 12:26:54 +0500 Subject: [PATCH] fix(hooks): stop precompact hook from blocking compaction (#856, #858) (#863) * fix(hooks): stop precompact hook from blocking compaction The precompact hook unconditionally returned {"decision": "block"}, which in Claude Code means "cancel compaction" with no retry mechanism. This made /compact permanently broken for all plugin users. Changed hook_precompact() to mine the transcript synchronously (so data lands before compaction) and return {"decision": "allow"}. This matches the standalone bash hook in hooks/ which already uses allow. Also extracted _get_mine_dir() and _mine_sync() helpers so precompact can mine from the transcript directory, not just MEMPAL_DIR. Stop hook behavior is unchanged -- left for #673 which implements the full silent save path. Closes #856, closes #858. * fix: use empty JSON instead of invalid \"allow\" decision value Claude Code only recognizes \"block\" as a top-level decision value. \"allow\" is a permissionDecision value for PreToolUse hooks, not a valid top-level decision. The correct way to not block is to return empty JSON. Caught by #872. --- mempalace/hooks_cli.py | 81 ++++++++++++++++++++++-------------- tests/test_hooks_cli.py | 91 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 131 insertions(+), 41 deletions(-) diff --git a/mempalace/hooks_cli.py b/mempalace/hooks_cli.py index 14fd9f7..d9512d6 100644 --- a/mempalace/hooks_cli.py +++ b/mempalace/hooks_cli.py @@ -122,20 +122,53 @@ def _output(data: dict): print(json.dumps(data, indent=2, ensure_ascii=False)) -def _maybe_auto_ingest(): - """If MEMPAL_DIR is set and exists, run mempalace mine in background.""" +def _get_mine_dir(transcript_path: str = "") -> str: + """Determine directory to mine from MEMPAL_DIR or transcript path.""" mempal_dir = os.environ.get("MEMPAL_DIR", "") if mempal_dir and os.path.isdir(mempal_dir): - try: - log_path = STATE_DIR / "hook.log" - with open(log_path, "a") as log_f: - subprocess.Popen( - [sys.executable, "-m", "mempalace", "mine", mempal_dir], - stdout=log_f, - stderr=log_f, - ) - except OSError: - pass + return mempal_dir + if transcript_path: + path = Path(transcript_path).expanduser() + if path.is_file(): + return str(path.parent) + return "" + + +def _maybe_auto_ingest(transcript_path: str = ""): + """Run mempalace mine in background if a mine directory is available.""" + mine_dir = _get_mine_dir(transcript_path) + if not mine_dir: + return + try: + STATE_DIR.mkdir(parents=True, exist_ok=True) + log_path = STATE_DIR / "hook.log" + with open(log_path, "a") as log_f: + subprocess.Popen( + [sys.executable, "-m", "mempalace", "mine", mine_dir], + stdout=log_f, + stderr=log_f, + ) + except OSError: + pass + + +def _mine_sync(transcript_path: str = ""): + """Run mempalace mine synchronously (for precompact -- data must land first).""" + mine_dir = _get_mine_dir(transcript_path) + if not mine_dir: + return + try: + STATE_DIR.mkdir(parents=True, exist_ok=True) + log_path = STATE_DIR / "hook.log" + with open(log_path, "a") as log_f: + subprocess.run( + [sys.executable, "-m", "mempalace", "mine", mine_dir], + stdout=log_f, + stderr=log_f, + timeout=60, + ) + except (OSError, subprocess.TimeoutExpired): + pass SUPPORTED_HARNESSES = {"claude-code", "codex"} @@ -192,7 +225,7 @@ def hook_stop(data: dict, harness: str): _log(f"TRIGGERING SAVE at exchange {exchange_count}") # Optional: auto-ingest if MEMPAL_DIR is set - _maybe_auto_ingest() + _maybe_auto_ingest(transcript_path) _output({"decision": "block", "reason": STOP_BLOCK_REASON}) else: @@ -214,29 +247,17 @@ def hook_session_start(data: dict, harness: str): def hook_precompact(data: dict, harness: str): - """Precompact hook: always block with comprehensive save instruction.""" + """Precompact hook: mine transcript synchronously, then allow compaction.""" parsed = _parse_harness_input(data, harness) session_id = parsed["session_id"] + transcript_path = parsed["transcript_path"] _log(f"PRE-COMPACT triggered for session {session_id}") - # Optional: auto-ingest synchronously before compaction (so memories land first) - mempal_dir = os.environ.get("MEMPAL_DIR", "") - if mempal_dir and os.path.isdir(mempal_dir): - try: - log_path = STATE_DIR / "hook.log" - with open(log_path, "a") as log_f: - subprocess.run( - [sys.executable, "-m", "mempalace", "mine", mempal_dir], - stdout=log_f, - stderr=log_f, - timeout=60, - ) - except OSError: - pass + # Mine synchronously so data lands before compaction proceeds + _mine_sync(transcript_path) - # Always block -- compaction = save everything - _output({"decision": "block", "reason": PRECOMPACT_BLOCK_REASON}) + _output({}) def run_hook(hook_name: str, harness: str): diff --git a/tests/test_hooks_cli.py b/tests/test_hooks_cli.py index 861c054..7bf6cf3 100644 --- a/tests/test_hooks_cli.py +++ b/tests/test_hooks_cli.py @@ -1,6 +1,7 @@ import contextlib import io import json +import subprocess from pathlib import Path from unittest.mock import patch @@ -9,8 +10,8 @@ import pytest from mempalace.hooks_cli import ( SAVE_INTERVAL, STOP_BLOCK_REASON, - PRECOMPACT_BLOCK_REASON, _count_human_messages, + _get_mine_dir, _log, _maybe_auto_ingest, _parse_harness_input, @@ -205,14 +206,13 @@ def test_session_start_passes_through(tmp_path): # --- hook_precompact --- -def test_precompact_always_blocks(tmp_path): +def test_precompact_allows(tmp_path): result = _capture_hook_output( hook_precompact, {"session_id": "test"}, state_dir=tmp_path, ) - assert result["decision"] == "block" - assert result["reason"] == PRECOMPACT_BLOCK_REASON + assert result == {} # --- _log --- @@ -238,7 +238,7 @@ def test_log_oserror_is_silenced(tmp_path): def test_maybe_auto_ingest_no_env(tmp_path): - """Without MEMPAL_DIR set, does nothing.""" + """Without MEMPAL_DIR or transcript_path, does nothing.""" with patch.dict("os.environ", {}, clear=True): with patch("mempalace.hooks_cli.STATE_DIR", tmp_path): _maybe_auto_ingest() # should not raise @@ -255,6 +255,17 @@ def test_maybe_auto_ingest_with_env(tmp_path): mock_popen.assert_called_once() +def test_maybe_auto_ingest_with_transcript(tmp_path): + """Falls back to transcript directory when MEMPAL_DIR is not set.""" + transcript = tmp_path / "t.jsonl" + transcript.write_text("") + with patch.dict("os.environ", {}, clear=True): + with patch("mempalace.hooks_cli.STATE_DIR", tmp_path): + with patch("mempalace.hooks_cli.subprocess.Popen") as mock_popen: + _maybe_auto_ingest(str(transcript)) + mock_popen.assert_called_once() + + def test_maybe_auto_ingest_oserror(tmp_path): """OSError during subprocess spawn is silenced.""" mempal_dir = tmp_path / "project" @@ -265,6 +276,33 @@ def test_maybe_auto_ingest_oserror(tmp_path): _maybe_auto_ingest() # should not raise +# --- _get_mine_dir --- + + +def test_get_mine_dir_mempal_dir(tmp_path): + """MEMPAL_DIR takes priority over transcript_path.""" + mempal_dir = tmp_path / "project" + mempal_dir.mkdir() + transcript = tmp_path / "t.jsonl" + transcript.write_text("") + with patch.dict("os.environ", {"MEMPAL_DIR": str(mempal_dir)}): + assert _get_mine_dir(str(transcript)) == str(mempal_dir) + + +def test_get_mine_dir_transcript_fallback(tmp_path): + """Falls back to transcript parent dir when MEMPAL_DIR is not set.""" + transcript = tmp_path / "t.jsonl" + transcript.write_text("") + with patch.dict("os.environ", {}, clear=True): + assert _get_mine_dir(str(transcript)) == str(tmp_path) + + +def test_get_mine_dir_empty(): + """Returns empty string when nothing is available.""" + with patch.dict("os.environ", {}, clear=True): + assert _get_mine_dir("") == "" + + # --- _parse_harness_input --- @@ -333,7 +371,7 @@ def test_stop_hook_oserror_on_write(tmp_path): def test_precompact_with_mempal_dir(tmp_path): - """Precompact runs subprocess.run when MEMPAL_DIR is set.""" + """Precompact runs subprocess.run (sync) when MEMPAL_DIR is set.""" mempal_dir = tmp_path / "project" mempal_dir.mkdir() with patch.dict("os.environ", {"MEMPAL_DIR": str(mempal_dir)}): @@ -343,7 +381,7 @@ def test_precompact_with_mempal_dir(tmp_path): {"session_id": "test"}, state_dir=tmp_path, ) - assert result["decision"] == "block" + assert result == {} mock_run.assert_called_once() @@ -358,7 +396,40 @@ def test_precompact_with_mempal_dir_oserror(tmp_path): {"session_id": "test"}, state_dir=tmp_path, ) - assert result["decision"] == "block" + assert result == {} + + +def test_precompact_with_timeout(tmp_path): + """Precompact handles TimeoutExpired gracefully -- still allows.""" + mempal_dir = tmp_path / "project" + mempal_dir.mkdir() + with patch.dict("os.environ", {"MEMPAL_DIR": str(mempal_dir)}): + with patch( + "mempalace.hooks_cli.subprocess.run", + side_effect=subprocess.TimeoutExpired(cmd="mine", timeout=60), + ): + result = _capture_hook_output( + hook_precompact, {"session_id": "test"}, state_dir=tmp_path + ) + assert result == {} + + +def test_precompact_mines_transcript_dir(tmp_path, monkeypatch): + """Precompact mines transcript directory when no MEMPAL_DIR.""" + transcript = tmp_path / "t.jsonl" + transcript.write_text("") + monkeypatch.delenv("MEMPAL_DIR", raising=False) + with patch("mempalace.hooks_cli.subprocess.run") as mock_run: + result = _capture_hook_output( + hook_precompact, + {"session_id": "test", "transcript_path": str(transcript)}, + state_dir=tmp_path, + ) + assert result == {} + mock_run.assert_called_once() + # Verify mine dir is the transcript's parent + call_args = mock_run.call_args[0][0] + assert str(tmp_path) in call_args[-1] # --- run_hook --- @@ -399,9 +470,7 @@ def test_run_hook_dispatches_precompact(tmp_path): with patch("mempalace.hooks_cli.STATE_DIR", tmp_path): with patch("mempalace.hooks_cli._output") as mock_output: run_hook("precompact", "claude-code") - mock_output.assert_called_once() - call_args = mock_output.call_args[0][0] - assert call_args["decision"] == "block" + mock_output.assert_called_once_with({}) def test_run_hook_unknown_hook():