Skip to content

Commit 6c2840c

Browse files
fix(hooks): real python-resolution for .sh hooks + regression tests
The legacy ``.sh`` hook scripts (``mempal_save_hook.sh``, ``mempal_precompact_hook.sh``) call ``python3 -m mempalace mine …`` directly. On macOS GUI launches of Claude Code — ``open -a``, Spotlight, the dock — the harness inherits ``PATH`` from launchd (``/usr/bin:/bin:/usr/sbin:/sbin``), which typically does not contain the Python where ``mempalace`` was installed. The hook then silently fails deep in the background log where users never look. The original drive-by fix (branch ``fix/hook-bugs``) replaced ``python3`` with ``$(command -v python3)``. Verified empirically that those two resolve to the same binary — the "fix" was a no-op, and the "nohup strips PATH" rationale in the commit message doesn't apply (the hooks don't use nohup). This PR delivers the fix the original branch intended: 1. **MEMPAL_PYTHON override** — users whose GUI-launch PATH lacks the right interpreter can export ``MEMPAL_PYTHON=/path/to/venv/python`` and the hooks pick it up first. Resolution order: $MEMPAL_PYTHON (if set and executable) → $(command -v python3) → bare ``python3`` 2. **Import sanity check before auto-ingest** — before running ``-m mempalace mine``, the hook runs ``-c 'import mempalace'``. On failure it logs a clear warning (with the fix instructions) to ``~/.mempalace/hook_state/hook.log`` and skips the ingest, preserving the Claude Code contract of returning 0 rather than crashing the Stop flow. 3. **Applied to all hook python calls** — not just the ingest line. The JSON parse and transcript-count calls use the same resolution so that ``MEMPAL_PYTHON`` consistently controls interpretation for the whole script. 4. **README** — adds a "Known Limitations" entry documenting the macOS GUI PATH behaviour and the ``MEMPAL_PYTHON`` workaround. (The "session restart after install" note was already on develop.) 5. **Regression tests** in ``tests/test_hooks_shell.py``: * ``MEMPAL_PYTHON`` override wins over PATH (proved via a marker-emitting shim that proxies to the real interpreter). * Non-executable ``MEMPAL_PYTHON`` falls back to PATH rather than crashing on permission denied. * Unset ``MEMPAL_PYTHON`` resolves via PATH. * Both hooks exit 0 and log the warning when the resolved interpreter can't import mempalace — Claude Code never sees a non-zero exit from the auto-ingest branch. Tests are skipped on Windows (POSIX-only bash scripts). 810/810 on Linux; the 5 new tests take ~0.2s total. ``hooks_cli.py`` (the Python implementation invoked via ``mempalace hook run …``) already uses ``sys.executable`` and is therefore trivially correct — no changes needed there. Supersedes abandoned branch ``fix/hook-bugs``. Co-Authored-By: MSL <232237854+milla-jovovich@users.noreply.github.com>
1 parent a74586c commit 6c2840c

4 files changed

Lines changed: 301 additions & 5 deletions

File tree

hooks/README.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,27 @@ Example output:
137137

138138
**Hooks require session restart after install.** Claude Code loads hooks from `settings.json` at session start only. If you run `mempalace init` or manually edit hook config mid-session, the hooks won't fire until you restart Claude Code. This is a Claude Code limitation.
139139

140+
**Hooks may need `MEMPAL_PYTHON` on macOS GUI launches.** When Claude Code (or any harness) is launched from a GUI — `open -a`, Spotlight, the dock — its `PATH` is the minimal `/usr/bin:/bin:/usr/sbin:/sbin` inherited from `launchd`, not your shell PATH. A `python3` found on that PATH is typically the system Python, which usually does not have `mempalace` installed. The hook logs a clear warning when this happens:
141+
142+
```
143+
WARN: /usr/bin/python3 cannot import mempalace — skipping auto-ingest. Set MEMPAL_PYTHON=/path/to/your/python to fix.
144+
```
145+
146+
To fix it, point the hook at the Python where you installed `mempalace`. For example, if you installed into a venv:
147+
148+
```bash
149+
# in your shell or the hook's environment
150+
export MEMPAL_PYTHON="$HOME/.venvs/mempalace/bin/python"
151+
```
152+
153+
Or, if you installed with `pipx` / `pip --user`:
154+
155+
```bash
156+
export MEMPAL_PYTHON="$(command -v mempalace | xargs -I{} dirname {})/python"
157+
```
158+
159+
Resolution priority inside the hook: `$MEMPAL_PYTHON` (if set and executable) → `$(command -v python3)` → bare `python3`.
160+
140161
## Cost
141162

142163
**Zero extra tokens.** The hooks notify the AI that saves happened in the background — the AI doesn't need to write anything in the chat. All filing is handled automatically. Previous versions asked the AI to write diary entries and drawer content in the chat window, which cost ~$1/session in retransmitted tokens.

hooks/mempal_precompact_hook.sh

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,18 +54,29 @@ mkdir -p "$STATE_DIR"
5454
# Leave empty to skip auto-ingest (AI handles saving via the block reason).
5555
MEMPAL_DIR=""
5656

57+
# Resolve the Python interpreter. Same contract as mempal_save_hook.sh:
58+
# MEMPAL_PYTHON (explicit override) → $(command -v python3) → bare python3.
59+
MEMPAL_PYTHON_BIN="${MEMPAL_PYTHON:-}"
60+
if [ -z "$MEMPAL_PYTHON_BIN" ] || [ ! -x "$MEMPAL_PYTHON_BIN" ]; then
61+
MEMPAL_PYTHON_BIN="$(command -v python3 2>/dev/null || echo python3)"
62+
fi
63+
5764
# Read JSON input from stdin
5865
INPUT=$(cat)
5966

60-
SESSION_ID=$(echo "$INPUT" | python3 -c "import sys,json; print(json.load(sys.stdin).get('session_id','unknown'))" 2>/dev/null)
67+
SESSION_ID=$(echo "$INPUT" | "$MEMPAL_PYTHON_BIN" -c "import sys,json; print(json.load(sys.stdin).get('session_id','unknown'))" 2>/dev/null)
6168

6269
echo "[$(date '+%H:%M:%S')] PRE-COMPACT triggered for session $SESSION_ID" >> "$STATE_DIR/hook.log"
6370

6471
# Optional: run mempalace ingest synchronously so memories land before compaction
6572
if [ -n "$MEMPAL_DIR" ] && [ -d "$MEMPAL_DIR" ]; then
6673
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
6774
REPO_DIR="$(dirname "$SCRIPT_DIR")"
68-
python3 -m mempalace mine "$MEMPAL_DIR" >> "$STATE_DIR/hook.log" 2>&1
75+
if ! "$MEMPAL_PYTHON_BIN" -c "import mempalace" 2>/dev/null; then
76+
echo "[$(date '+%H:%M:%S')] WARN: $MEMPAL_PYTHON_BIN cannot import mempalace — skipping pre-compact ingest. Set MEMPAL_PYTHON=/path/to/your/python to fix." >> "$STATE_DIR/hook.log"
77+
else
78+
"$MEMPAL_PYTHON_BIN" -m mempalace mine "$MEMPAL_DIR" >> "$STATE_DIR/hook.log" 2>&1
79+
fi
6980
fi
7081

7182
# Notify — compaction is about to happen but filing is handled in background

hooks/mempal_save_hook.sh

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,28 @@ mkdir -p "$STATE_DIR"
6161
# Leave empty to skip auto-ingest (AI handles saving via the block reason).
6262
MEMPAL_DIR=""
6363

64+
# Resolve the Python interpreter the hook should use.
65+
#
66+
# Why this is nontrivial: GUI-launched Claude Code on macOS (or any harness
67+
# that doesn't inherit the user's shell PATH) may find a `python3` on PATH
68+
# that lacks mempalace — e.g. /usr/bin/python3 while the user installed
69+
# mempalace into a venv or pyenv. Users in that situation can point the
70+
# hook at the right interpreter by exporting MEMPAL_PYTHON.
71+
#
72+
# Resolution order (first hit wins):
73+
# 1. $MEMPAL_PYTHON — explicit user override (absolute path)
74+
# 2. $(command -v python3) — first python3 on the hook's PATH
75+
# 3. bare "python3" — last-resort fallback (hope the PATH has it)
76+
MEMPAL_PYTHON_BIN="${MEMPAL_PYTHON:-}"
77+
if [ -z "$MEMPAL_PYTHON_BIN" ] || [ ! -x "$MEMPAL_PYTHON_BIN" ]; then
78+
MEMPAL_PYTHON_BIN="$(command -v python3 2>/dev/null || echo python3)"
79+
fi
80+
6481
# Read JSON input from stdin
6582
INPUT=$(cat)
6683

6784
# Parse all fields in a single Python call (3x faster than separate invocations)
68-
eval $(echo "$INPUT" | python3 -c "
85+
eval $(echo "$INPUT" | "$MEMPAL_PYTHON_BIN" -c "
6986
import sys, json
7087
data = json.load(sys.stdin)
7188
sid = data.get('session_id', 'unknown')
@@ -92,7 +109,7 @@ fi
92109
# Count human messages in the JSONL transcript
93110
# SECURITY: Pass transcript path as sys.argv to avoid shell injection via crafted paths
94111
if [ -f "$TRANSCRIPT_PATH" ]; then
95-
EXCHANGE_COUNT=$(python3 - "$TRANSCRIPT_PATH" <<'PYEOF'
112+
EXCHANGE_COUNT=$("$MEMPAL_PYTHON_BIN" - "$TRANSCRIPT_PATH" <<'PYEOF'
96113
import json, sys
97114
count = 0
98115
with open(sys.argv[1]) as f:
@@ -137,7 +154,14 @@ if [ "$SINCE_LAST" -ge "$SAVE_INTERVAL" ] && [ "$EXCHANGE_COUNT" -gt 0 ]; then
137154
if [ -n "$MEMPAL_DIR" ] && [ -d "$MEMPAL_DIR" ]; then
138155
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
139156
REPO_DIR="$(dirname "$SCRIPT_DIR")"
140-
python3 -m mempalace mine "$MEMPAL_DIR" >> "$STATE_DIR/hook.log" 2>&1 &
157+
# Sanity-check the resolved interpreter can import mempalace before
158+
# we fire-and-forget. Without this, a bad PATH results in silent
159+
# failures logged deep in hook.log where users never look.
160+
if ! "$MEMPAL_PYTHON_BIN" -c "import mempalace" 2>/dev/null; then
161+
echo "[$(date '+%H:%M:%S')] WARN: $MEMPAL_PYTHON_BIN cannot import mempalace — skipping auto-ingest. Set MEMPAL_PYTHON=/path/to/your/python to fix." >> "$STATE_DIR/hook.log"
162+
else
163+
"$MEMPAL_PYTHON_BIN" -m mempalace mine "$MEMPAL_DIR" >> "$STATE_DIR/hook.log" 2>&1 &
164+
fi
141165
fi
142166

143167
# Notify the AI that a checkpoint happened — but do NOT ask it to write

tests/test_hooks_shell.py

Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,240 @@
1+
"""
2+
Integration tests for the legacy ``.sh`` hook scripts.
3+
4+
The shell hooks do their own Python resolution (unlike the Python
5+
``hooks_cli.py`` which uses ``sys.executable`` — trivially correct).
6+
GUI-launched harnesses on macOS provide a minimal PATH that often lacks
7+
the Python where ``mempalace`` is installed, so the shell path needs to:
8+
9+
1. honour ``$MEMPAL_PYTHON`` as an explicit user override;
10+
2. fall back to ``$(command -v python3)`` / bare ``python3``;
11+
3. *never* crash the hook when the resolved interpreter can't import
12+
mempalace — log and skip the auto-ingest instead, so Claude Code
13+
doesn't see a non-zero exit from its Stop hook.
14+
15+
These regressions matter because every failure mode they catch produced
16+
silent breakage in production — the user's hook appeared to "not fire"
17+
but was actually crashing deep in a PATH-resolution edge case.
18+
"""
19+
20+
from __future__ import annotations
21+
22+
import json
23+
import os
24+
import stat
25+
import subprocess
26+
import sys
27+
from pathlib import Path
28+
29+
import pytest
30+
31+
REPO_ROOT = Path(__file__).resolve().parent.parent
32+
SAVE_HOOK = REPO_ROOT / "hooks" / "mempal_save_hook.sh"
33+
PRECOMPACT_HOOK = REPO_ROOT / "hooks" / "mempal_precompact_hook.sh"
34+
35+
36+
pytestmark = pytest.mark.skipif(os.name == "nt", reason="bash hook scripts are POSIX-only")
37+
38+
39+
# ── helpers ───────────────────────────────────────────────────────────────
40+
41+
42+
def _write_fake_python(
43+
path: Path, *, can_import_mempalace: bool = False, marker_file: Path | None = None
44+
) -> Path:
45+
"""Create a python3 shim that proxies to the real interpreter so
46+
the hook's JSON-parsing calls still work, but fails ``-c 'import
47+
mempalace'`` / ``-m mempalace`` when ``can_import_mempalace`` is
48+
False.
49+
50+
Every invocation appends the shim name to ``marker_file`` so tests
51+
can prove which interpreter the hook invoked — using a file because
52+
the hook pipes some python calls to ``2>/dev/null``, so stderr
53+
markers are unreliable."""
54+
real_python = sys.executable
55+
marker = str(marker_file) if marker_file is not None else ""
56+
shim_src = f"""#!/bin/bash
57+
# Fake python3 shim: proxy to the real interpreter, drop a marker,
58+
# and simulate a missing mempalace install when configured that way.
59+
MARKER_FILE="{marker}"
60+
if [ -n "$MARKER_FILE" ]; then
61+
echo "{path.name}" >> "$MARKER_FILE"
62+
fi
63+
CAN_IMPORT={"1" if can_import_mempalace else "0"}
64+
# Simulate the "mempalace is not installed in this interpreter" case.
65+
if [ "$CAN_IMPORT" = "0" ]; then
66+
if [ "$1" = "-c" ] && echo "$2" | grep -q "import mempalace"; then
67+
exit 1
68+
fi
69+
if [ "$1" = "-m" ] && [ "$2" = "mempalace" ]; then
70+
exit 1
71+
fi
72+
fi
73+
# Everything else — JSON parsing, heredoc stdin, etc — delegate to real python.
74+
exec "{real_python}" "$@"
75+
"""
76+
path.write_text(shim_src)
77+
path.chmod(path.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
78+
return path
79+
80+
81+
def _run_hook(
82+
script: Path,
83+
stdin_json: dict,
84+
*,
85+
env_overrides: dict | None = None,
86+
path_prefix: list[Path] | None = None,
87+
) -> subprocess.CompletedProcess:
88+
"""Invoke a shell hook with a minimal controlled environment."""
89+
env = {
90+
# Give the hook a clean slate — no inherited MEMPAL_* vars.
91+
"HOME": os.environ.get("HOME", "/tmp"),
92+
"PATH": os.environ.get("PATH", "/usr/bin:/bin"),
93+
}
94+
if path_prefix:
95+
env["PATH"] = os.pathsep.join(str(p) for p in path_prefix) + os.pathsep + env["PATH"]
96+
if env_overrides:
97+
env.update(env_overrides)
98+
return subprocess.run(
99+
["bash", str(script)],
100+
input=json.dumps(stdin_json),
101+
capture_output=True,
102+
text=True,
103+
env=env,
104+
timeout=30,
105+
)
106+
107+
108+
# ── MEMPAL_PYTHON resolution contract ────────────────────────────────────
109+
110+
111+
class TestMempalPythonOverride:
112+
def test_explicit_override_wins_over_path(self, tmp_path):
113+
"""If MEMPAL_PYTHON is set and executable, the hook must use it
114+
in preference to whatever is on PATH."""
115+
marker = tmp_path / "markers.log"
116+
fake = _write_fake_python(
117+
tmp_path / "override_python",
118+
can_import_mempalace=True,
119+
marker_file=marker,
120+
)
121+
result = _run_hook(
122+
SAVE_HOOK,
123+
{"session_id": "abc", "stop_hook_active": False, "transcript_path": ""},
124+
env_overrides={"MEMPAL_PYTHON": str(fake), "HOME": str(tmp_path)},
125+
)
126+
assert (
127+
result.returncode == 0
128+
), f"hook exited non-zero: stderr={result.stderr!r} stdout={result.stdout!r}"
129+
invocations = marker.read_text().splitlines() if marker.exists() else []
130+
assert (
131+
"override_python" in invocations
132+
), f"MEMPAL_PYTHON override was not used. Marker log: {invocations!r}"
133+
134+
def test_ignores_override_when_not_executable(self, tmp_path):
135+
"""If MEMPAL_PYTHON is set but the file isn't executable, the
136+
hook must fall back to PATH rather than blow up with a
137+
'permission denied'."""
138+
bogus = tmp_path / "not_executable"
139+
bogus.write_text("# not a python")
140+
# Do NOT chmod +x — the hook should notice and skip.
141+
result = _run_hook(
142+
SAVE_HOOK,
143+
{"session_id": "abc", "stop_hook_active": False, "transcript_path": ""},
144+
env_overrides={"MEMPAL_PYTHON": str(bogus), "HOME": str(tmp_path)},
145+
)
146+
assert (
147+
result.returncode == 0
148+
), f"hook crashed on non-executable MEMPAL_PYTHON: {result.stderr!r}"
149+
150+
def test_falls_back_to_path_when_unset(self, tmp_path):
151+
"""With MEMPAL_PYTHON unset, the hook uses whatever ``python3``
152+
is found on PATH. Prove this by putting a marker-emitting shim
153+
first on PATH."""
154+
marker = tmp_path / "markers.log"
155+
fake = _write_fake_python(
156+
tmp_path / "python3",
157+
can_import_mempalace=True,
158+
marker_file=marker,
159+
)
160+
result = _run_hook(
161+
SAVE_HOOK,
162+
{"session_id": "abc", "stop_hook_active": False, "transcript_path": ""},
163+
env_overrides={"MEMPAL_PYTHON": "", "HOME": str(tmp_path)},
164+
path_prefix=[fake.parent],
165+
)
166+
assert result.returncode == 0
167+
invocations = marker.read_text().splitlines() if marker.exists() else []
168+
assert (
169+
"python3" in invocations
170+
), f"fallback-to-PATH did not use the shimmed python3. Marker log: {invocations!r}"
171+
172+
173+
# ── graceful degradation when mempalace can't be imported ────────────────
174+
175+
176+
class TestGracefulDegradation:
177+
def test_save_hook_does_not_crash_when_mempalace_missing(self, tmp_path):
178+
"""A fake python3 that fails ``import mempalace`` must still
179+
leave the hook exiting 0 — the hook logs a warning instead of
180+
crashing Claude Code's Stop flow."""
181+
fake = _write_fake_python(tmp_path / "noimport_python", can_import_mempalace=False)
182+
# Seed a transcript the hook can count (needs MEMPAL_DIR set so
183+
# the auto-ingest branch is reached, which is where the import
184+
# check lives).
185+
mempal_dir = tmp_path / "mempal"
186+
mempal_dir.mkdir()
187+
# Rewrite the hook with our MEMPAL_DIR baked in via a temporary
188+
# copy, since MEMPAL_DIR is a script-local variable, not an env.
189+
patched = tmp_path / "patched_save.sh"
190+
original = SAVE_HOOK.read_text()
191+
patched.write_text(original.replace('MEMPAL_DIR=""', f'MEMPAL_DIR="{mempal_dir}"'))
192+
patched.chmod(0o755)
193+
194+
# Write a transcript with enough messages to trigger a save.
195+
transcript = tmp_path / "transcript.jsonl"
196+
with open(transcript, "w") as f:
197+
for _ in range(30): # well above SAVE_INTERVAL=15
198+
f.write(json.dumps({"message": {"role": "user", "content": "hi"}}) + "\n")
199+
200+
result = _run_hook(
201+
patched,
202+
{
203+
"session_id": "abc",
204+
"stop_hook_active": False,
205+
"transcript_path": str(transcript),
206+
},
207+
env_overrides={"MEMPAL_PYTHON": str(fake), "HOME": str(tmp_path)},
208+
)
209+
# Hook exits 0 even when mempalace is missing.
210+
assert (
211+
result.returncode == 0
212+
), f"hook crashed when mempalace could not be imported: {result.stderr!r}"
213+
# Warning went to the hook log, not to the AI.
214+
hook_log = tmp_path / ".mempalace" / "hook_state" / "hook.log"
215+
if hook_log.exists():
216+
log_text = hook_log.read_text()
217+
assert (
218+
"cannot import mempalace" in log_text
219+
), f"expected a 'cannot import mempalace' warning in hook.log, got:\n{log_text}"
220+
221+
def test_precompact_hook_does_not_crash_when_mempalace_missing(self, tmp_path):
222+
"""Same contract as the save hook — a broken interpreter must
223+
not propagate as a non-zero exit to the harness."""
224+
fake = _write_fake_python(tmp_path / "noimport_python", can_import_mempalace=False)
225+
mempal_dir = tmp_path / "mempal"
226+
mempal_dir.mkdir()
227+
patched = tmp_path / "patched_precompact.sh"
228+
original = PRECOMPACT_HOOK.read_text()
229+
patched.write_text(original.replace('MEMPAL_DIR=""', f'MEMPAL_DIR="{mempal_dir}"'))
230+
patched.chmod(0o755)
231+
232+
result = _run_hook(
233+
patched,
234+
{"session_id": "abc"},
235+
env_overrides={"MEMPAL_PYTHON": str(fake), "HOME": str(tmp_path)},
236+
)
237+
assert result.returncode == 0, f"precompact hook crashed: {result.stderr!r}"
238+
hook_log = tmp_path / ".mempalace" / "hook_state" / "hook.log"
239+
if hook_log.exists():
240+
assert "cannot import mempalace" in hook_log.read_text()

0 commit comments

Comments
 (0)