diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9e2510f..3e9beab 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -6,6 +6,12 @@ All notable changes to Palinode. Format follows [Keep a Changelog](https://keepa ### Added +- `doctor`: new `memory_dir_writable` check. `memory_dir_exists` is critical but only + asserts the path is a directory, so a memory directory on a read-only mount, owned by + another user, or with its mode tightened by hand reported green while every save failed. + The new check tests `os.W_OK | os.X_OK` and carries remediation naming those three + causes. ([#206](https://github.com/phasespace-labs/palinode/issues/206)) + ### Changed ### Fixed diff --git a/docs/DOCTOR.md b/docs/DOCTOR.md index c24992d..c174c22 100644 --- a/docs/DOCTOR.md +++ b/docs/DOCTOR.md @@ -49,6 +49,7 @@ Pure-disk checks. Cheap, no network. | Check | Severity ceiling | Catches | |---|---|---| | `memory_dir_exists` | critical | `PALINODE_DIR` points at a missing or non-directory path | +| `memory_dir_writable` | critical | `memory_dir` exists but the running user cannot create files in it | | `db_path_resolvable` | error | `db_path` parent missing, or the file is not openable as SQLite | | `db_path_under_memory_dir` | warn | `db_path` resolves outside `memory_dir` (the rename-drift signature) | | `phantom_db_files` | critical | One or more `.palinode.db` files exist outside the configured path | @@ -58,6 +59,21 @@ Pure-disk checks. Cheap, no network. Verifies `Path(config.memory_dir)` exists and is a directory. Without it nothing else works, so this is the single critical-severity gate. Failure prints the resolved path and the `mkdir -p` command. **Fixable via `--fix`** (creates the directory). +#### `memory_dir_writable` + +Verifies the running user can create files in `config.memory_dir`. A directory that exists +but is not writable passes `memory_dir_exists` and then fails every save, which is what a +read-only mount, an ownership mismatch, or a hand-tightened mode all look like. + +Checks `os.W_OK | os.X_OK`, because creating a file inside a directory needs search +permission as well as write. Uses `os.access` rather than writing a probe file, matching +`audit_log_writable`; that is advisory and can disagree with a real write, but doctor should +not leave files in your memory directory to answer a question. + +When `memory_dir` is absent or is not a directory the check passes with a message saying it +did not apply. `memory_dir_exists` already reports that case and one cause should not +produce two failures. Not fixable via `--fix`. + #### `db_path_resolvable` Verifies the configured `db_path` is openable by SQLite in read-only mode and that its parent directory exists. Uses `PRAGMA schema_version` to validate the SQLite header, which catches a non-SQLite file masquerading as the DB. Full `PRAGMA integrity_check` is left to deeper future checks because it can be slow on large stores. diff --git a/palinode/diagnostics/checks/memory_dir.py b/palinode/diagnostics/checks/memory_dir.py index 4b5f854..98c5f48 100644 --- a/palinode/diagnostics/checks/memory_dir.py +++ b/palinode/diagnostics/checks/memory_dir.py @@ -1,11 +1,13 @@ """ -Check: memory_dir_exists +Checks: memory_dir_exists, memory_dir_writable -Verifies that the configured memory directory is present on disk. +Verifies that the configured memory directory is present on disk and that +the running user can create files inside it. Severity: critical — without it nothing works. """ from __future__ import annotations +import os from pathlib import Path from palinode.diagnostics.registry import register @@ -36,3 +38,60 @@ def memory_dir_exists(ctx: DoctorContext) -> CheckResult: f" mkdir -p {memory_dir}" ), ) + + +@register(tags=("fast",)) +def memory_dir_writable(ctx: DoctorContext) -> CheckResult: + """Verify that the running user can create files in config.memory_dir. + + A memory directory that exists but is not writable passes + ``memory_dir_exists`` and then fails every save. Writing a file needs + search permission on the directory as well as write permission, so both + bits are required here. + + Uses ``os.access`` rather than a write probe, matching + ``audit_log_writable``. It is advisory and can disagree with a real + write, but doctor should not leave files behind in the user's memory + directory to answer a question. + """ + memory_dir = Path(ctx.config.memory_dir).expanduser().resolve() + + if not (memory_dir.exists() and memory_dir.is_dir()): + # memory_dir_exists already reports this. One cause, one failure. + return CheckResult( + name="memory_dir_writable", + severity="critical", + passed=True, + message=( + f"Memory directory is absent, so writability does not apply: " + f"{memory_dir}" + ), + remediation=None, + ) + + if os.access(str(memory_dir), os.W_OK | os.X_OK): + return CheckResult( + name="memory_dir_writable", + severity="critical", + passed=True, + message=f"Memory directory is writable: {memory_dir}", + remediation=None, + ) + + return CheckResult( + name="memory_dir_writable", + severity="critical", + passed=False, + message=( + f"Memory directory exists but is not writable: {memory_dir} " + "(Every save will fail.)" + ), + remediation=( + f"Check ownership, whether the path is a read-only mount, and the\n" + f"directory mode. To fix a mode that was tightened by hand:\n" + f" chmod u+wx {memory_dir}\n" + f"To fix an ownership mismatch:\n" + f" chown $(id -u):$(id -g) {memory_dir}\n" + f"Or set PALINODE_DIR to a writable path." + ), + ) diff --git a/tests/test_doctor_memory_dir_writable.py b/tests/test_doctor_memory_dir_writable.py new file mode 100644 index 0000000..333039f --- /dev/null +++ b/tests/test_doctor_memory_dir_writable.py @@ -0,0 +1,114 @@ +""" +Tests for the memory_dir_writable doctor check. + +Covers: + - writable directory passes + - existing directory with write permission removed fails, with remediation + - directory with write but no search permission fails + - absent directory returns the informational pass, not a second failure + - a file where the directory should be takes the same informational pass + +Real directories under tmp_path, real mode bits. No mocking of os.access. +""" +from __future__ import annotations + +import os +import stat +import sys +from pathlib import Path + +import pytest + +from palinode.core.config import Config +from palinode.diagnostics.runner import run_one +from palinode.diagnostics.types import DoctorContext + + +def _ctx(memory_dir: Path) -> DoctorContext: + """Build a DoctorContext pointed at *memory_dir*.""" + cfg = Config( + memory_dir=str(memory_dir), + db_path=str(memory_dir / ".palinode.db"), + ) + cfg.doctor.search_roots = [str(memory_dir)] + return DoctorContext(config=cfg) + + +def test_writable_directory_passes(tmp_path: Path) -> None: + memory_dir = tmp_path / "palinode" + memory_dir.mkdir() + + result = run_one(_ctx(memory_dir), "memory_dir_writable") + + assert result.passed is True + assert result.severity == "critical" + assert result.remediation is None + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="POSIX mode bits do not govern directory writability on Windows", +) +@pytest.mark.skipif( + hasattr(os, "geteuid") and os.geteuid() == 0, + reason="os.access reports success for root regardless of mode", +) +def test_unwritable_directory_fails_with_remediation(tmp_path: Path) -> None: + memory_dir = tmp_path / "palinode" + memory_dir.mkdir() + original = stat.S_IMODE(memory_dir.stat().st_mode) + memory_dir.chmod(stat.S_IRUSR | stat.S_IXUSR) + try: + result = run_one(_ctx(memory_dir), "memory_dir_writable") + finally: + # Without this, tmp_path cleanup fails on some platforms. + memory_dir.chmod(original) + + assert result.passed is False + assert result.severity == "critical" + assert result.remediation is not None + assert str(memory_dir) in result.message + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="POSIX mode bits do not govern directory writability on Windows", +) +@pytest.mark.skipif( + hasattr(os, "geteuid") and os.geteuid() == 0, + reason="os.access reports success for root regardless of mode", +) +def test_directory_without_search_permission_fails(tmp_path: Path) -> None: + """Write permission alone is not enough to create a file inside.""" + memory_dir = tmp_path / "palinode" + memory_dir.mkdir() + original = stat.S_IMODE(memory_dir.stat().st_mode) + memory_dir.chmod(stat.S_IRUSR | stat.S_IWUSR) + try: + result = run_one(_ctx(memory_dir), "memory_dir_writable") + finally: + memory_dir.chmod(original) + + assert result.passed is False + + +def test_absent_directory_reports_an_informational_pass(tmp_path: Path) -> None: + """memory_dir_exists owns the absent case, so this one stays quiet.""" + memory_dir = tmp_path / "does-not-exist" + + result = run_one(_ctx(memory_dir), "memory_dir_writable") + + assert result.passed is True + assert result.remediation is None + assert "does not apply" in result.message + + +def test_file_where_the_directory_should_be_is_not_a_second_failure( + tmp_path: Path, +) -> None: + memory_dir = tmp_path / "palinode" + memory_dir.write_text("not a directory") + + result = run_one(_ctx(memory_dir), "memory_dir_writable") + + assert result.passed is True