Skip to content

doctor: check that memory_dir is writable - #213

Merged
Paul-Kyle merged 3 commits into
phasespace-labs:mainfrom
chiruu12:fix/206-memory-dir-writable
Sep 15, 2026
Merged

Paul-Kyle merged 3 commits into
phasespace-labs:mainfrom
chiruu12:fix/206-memory-dir-writable

Conversation

@chiruu12

Copy link
Copy Markdown
Contributor

Closes #206.

memory_dir_exists is registered critical and only asserts the path exists and is a
directory. A memory directory that exists but cannot be written to passes it, and then
every save fails. Confirmed on a directory with the write bit cleared:

memory_dir_exists: passed=True sev=critical
real write:        PermissionError

That is the state produced by a read-only bind mount, a volume whose ownership does not
match the running user, or a mode tightened by hand.

This adds a separate memory_dir_writable check rather than widening the existing one,
so the old name keeps describing what it does and the new failure gets its own message
and remediation. Same shape as audit_log_writable: os.access, no write probe.

Two details worth flagging.

It tests os.W_OK | os.X_OK, not W_OK alone. Creating a file inside a directory needs
search permission as well as write, so a directory that is -w- is unusable in practice.
audit_log_writable checks W_OK alone on its parent and has the same gap; left alone
here, one check per PR.

An absent or non-directory path is a defined pass, not a skip, with a message saying the
check did not apply. memory_dir_exists already reports that case and one cause should
not produce two failures.

Five tests, on real directories and real mode bits. The permission tests restore the mode
in a finally, skip on Windows, and skip under root, where os.access reports success
regardless of mode.

Docs: a row and a section in DOCTOR.md, and a CHANGELOG entry under Unreleased.

No --fix action. Registering one needs a fixes.py whitelist edit, and making a user's
memory directory writable is not a change the tool should make on its own.

Copilot AI lite review requested due to automatic review settings September 15, 2026 05:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Paul-Kyle
Paul-Kyle merged commit accd9db into phasespace-labs:main Sep 15, 2026
13 checks passed
@Paul-Kyle

Copy link
Copy Markdown
Member

Merged — thank you, and sorry it sat as long as it did.

Three things I want to name. Adding a check rather than widening memory_dir_exists keeps the old name honest and gives the new failure its own message, which is the difference between a doctor that reports and one that explains. Requiring os.X_OK alongside W_OK is correct and is the part a reviewer would have missed — a directory you can write to but not search is unusable, and you found that audit_log_writable has the same gap and left it alone anyway. One check per PR is the right instinct; open that one when you want it.

And skipping the permission tests under root is the detail people forget: os.access reports success there regardless of mode, so without the skip those two tests fail in any root container and the failure reads as a problem with the check rather than with the environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

doctor: memory_dir_exists is critical but never checks the directory is writable

3 participants