fix(hooks): use is_dir() for palace root check (review feedback)
Both @igorls and the Qodo bot flagged that `_palace_root_exists()` used `Path.exists()`, which returns True for a regular file. A stray file at `~/.mempalace` would let the kill-switch be bypassed and crash later in `STATE_DIR.mkdir()` with NotADirectoryError. Switched to `Path.is_dir()`. Also fold `_log()`'s inline check through `_palace_root_exists()` so both kill-switch sites use the same predicate. New test pins the behavior: a regular file at the palace root path is treated as absent (hook short-circuits, _log does not crash, the stray file is left untouched).
This commit is contained in:
@@ -26,8 +26,13 @@ def _palace_root_exists() -> bool:
|
|||||||
All hook side effects (logging, state dir creation, mining, ingestion)
|
All hook side effects (logging, state dir creation, mining, ingestion)
|
||||||
must respect this and short-circuit BEFORE touching disk — including
|
must respect this and short-circuit BEFORE touching disk — including
|
||||||
before logging the short-circuit itself.
|
before logging the short-circuit itself.
|
||||||
|
|
||||||
|
Uses ``is_dir()`` rather than ``exists()`` so a stray regular file at
|
||||||
|
``~/.mempalace`` (or a broken symlink) is treated as absent — otherwise
|
||||||
|
the kill-switch would be bypassed and ``STATE_DIR.mkdir()`` would later
|
||||||
|
crash on ``NotADirectoryError``.
|
||||||
"""
|
"""
|
||||||
return PALACE_ROOT.exists()
|
return PALACE_ROOT.is_dir()
|
||||||
|
|
||||||
|
|
||||||
def _mempalace_python() -> str:
|
def _mempalace_python() -> str:
|
||||||
@@ -154,7 +159,7 @@ _state_dir_initialized = False
|
|||||||
|
|
||||||
def _log(message: str):
|
def _log(message: str):
|
||||||
"""Append to hook state log file."""
|
"""Append to hook state log file."""
|
||||||
if not PALACE_ROOT.exists():
|
if not _palace_root_exists():
|
||||||
return # User removed the palace; do not recreate by logging
|
return # User removed the palace; do not recreate by logging
|
||||||
global _state_dir_initialized
|
global _state_dir_initialized
|
||||||
try:
|
try:
|
||||||
|
|||||||
@@ -1035,3 +1035,35 @@ def test_existing_dir_proceeds_normally(tmp_path, monkeypatch):
|
|||||||
# _log should have created the state dir under the existing palace root
|
# _log should have created the state dir under the existing palace root
|
||||||
assert (fake_root / "hook_state").exists()
|
assert (fake_root / "hook_state").exists()
|
||||||
assert (fake_root / "hook_state" / "hook.log").is_file()
|
assert (fake_root / "hook_state" / "hook.log").is_file()
|
||||||
|
|
||||||
|
|
||||||
|
def test_regular_file_at_palace_root_treated_as_absent(tmp_path, monkeypatch):
|
||||||
|
"""A regular file at ~/.mempalace must be treated the same as absent.
|
||||||
|
|
||||||
|
``Path.exists()`` returns True for a regular file, which would let the
|
||||||
|
kill-switch be bypassed and crash later when ``STATE_DIR.mkdir()`` runs
|
||||||
|
on ``NotADirectoryError``. ``_palace_root_exists()`` must use
|
||||||
|
``is_dir()`` so a stray file (or broken symlink) short-circuits cleanly.
|
||||||
|
"""
|
||||||
|
fake_root = tmp_path / "file-not-dir"
|
||||||
|
fake_root.write_text("oops, this is a file not a directory")
|
||||||
|
monkeypatch.setattr(hooks_cli_mod, "PALACE_ROOT", fake_root)
|
||||||
|
monkeypatch.setattr(hooks_cli_mod, "STATE_DIR", fake_root / "hook_state")
|
||||||
|
monkeypatch.setattr(hooks_cli_mod, "_state_dir_initialized", False)
|
||||||
|
|
||||||
|
# _palace_root_exists() is the source of truth — it must return False.
|
||||||
|
assert hooks_cli_mod._palace_root_exists() is False
|
||||||
|
|
||||||
|
# Hooks must short-circuit (return {} on stdout) and not touch disk.
|
||||||
|
buf = io.StringIO()
|
||||||
|
with contextlib.redirect_stdout(buf):
|
||||||
|
hook_session_start({"session_id": "file-at-root"}, "claude-code")
|
||||||
|
assert json.loads(buf.getvalue() or "{}") == {}
|
||||||
|
|
||||||
|
# _log must also short-circuit — it must NOT try to mkdir a path under a
|
||||||
|
# regular file (which would raise NotADirectoryError).
|
||||||
|
_log("test message") # would raise if not short-circuited
|
||||||
|
|
||||||
|
# The stray file is left untouched; we never try to convert it.
|
||||||
|
assert fake_root.is_file()
|
||||||
|
assert fake_root.read_text() == "oops, this is a file not a directory"
|
||||||
|
|||||||
Reference in New Issue
Block a user