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.

Author: mvalentsev

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):

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 @@
 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 bad_write_text(*args, **kwargs):
 
 
 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():