Claude finished @groksrc's task in 2m 35s —— View job

PR Review: fix(core): honor BASIC_MEMORY_CONFIG_DIR across remaining call sites

• Gather context and read changed files
• Review source code changes
• Review test coverage
• Verify checklist items
• Post final review

Summary

This is a clean, well-scoped follow-up to #743. Each of the five hardcoded Path.home() / ".basic-memory" call sites has been corrected with a minimal, targeted change. The approach is consistent across all sites: use resolve_data_dir() when no config object is in scope, or use app_config.data_dir_path / config_manager.config.data_dir_path when one is already threaded through.

Overall: LGTM with minor feedback below.

Code Quality Checklist

Findings

1. Stale docstring — utils.py:265

The setup_logging docstring was not updated alongside the implementation:

# utils.py:265 — still says the old path
log_to_file: Write to ~/.basic-memory/basic-memory.log with rotation

Should be <basic-memory data dir>/basic-memory.log (or reference BASIC_MEMORY_CONFIG_DIR). Fix this →

2. Stale docstring — ignore_utils.py:183

load_gitignore_patterns still references the old path:

# ignore_utils.py:183
1. ~/.basic-memory/.bmignore (user's global ignore patterns)

Since get_bmignore_path() was updated, this docstring should read <basic-memory data dir>/.bmignore. Fix this →

3. # pragma: no cover may now be removable — project_service.py:1142–1146

if watch_status_path.exists():
    try:  # pragma: no cover
        watch_status = json.loads(  # pragma: no cover
            watch_status_path.read_text(encoding="utf-8")
        )
    except Exception:  # pragma: no cover
        pass

The new test test_get_system_status_reads_watch_status_from_config_dir creates the watch-status file and calls get_system_status(), so this try block is now exercised. If coverage is confirming these lines are hit, removing the # pragma: no cover annotations would keep the "100% coverage" claim accurate and self-consistent. Worth checking whether removing them keeps coverage at 100%.

4. Minor — remaining ~/.basic-memory references in bisync_commands.py docstrings (lines 48–60, 104)

These are comments only (the underlying calls to get_bmignore_path() are now correct), so there's no functional impact. Worth a follow-up cleanup pass but not blocking.

Correctness Notes

• Circular import handled correctly. config.py imports from utils.py, so the deferred from basic_memory.config import resolve_data_dir inside setup_logging is the right approach. Top-level imports in ignore_utils.py and rclone_commands.py are safe because config.py doesn't import those modules.
• WatchService.status_path evaluated at init-time. Correct — the path is resolved once when the service starts, which is the intended behavior.
• data_dir_path property calls resolve_data_dir() dynamically. Confirmed — no caching concern; the env var is read fresh on each property access, so monkeypatching in tests works reliably.
• Test for get_system_status uses @pytest.mark.asyncio on a synchronous method. Fine — async tests can call sync methods, and the project_service fixture is async.

Claude finished @groksrc's task in 2m 47s —— View job

PR Review: fix(core): honor BASIC_MEMORY_CONFIG_DIR across remaining call sites

• Gather context
• Read changed source files
• Check previous findings status
• Post final review

What changed in the latest commit (99af4ea)

The new commit makes test_setup_logging_honors_basic_memory_config_dir platform-agnostic:

• Removed monkeypatch.setattr(utils.os, "name", "posix") — no longer forces POSIX branch
• Changed assertion from an exact filename match (basic-memory.log) to directory + prefix + suffix checks, which works on both Windows (per-process basic-memory-<pid>.log) and POSIX (shared basic-memory.log)

This is the correct fix — the original test was broken on Windows because it hardcoded the POSIX filename while forcing os.name.

Code Quality Checklist

Outstanding Findings (3 items, carried from previous reviews — not yet addressed)

1. Stale docstring — utils.py:265

# Current (stale)
log_to_file: Write to ~/.basic-memory/basic-memory.log with rotation

# Should be
log_to_file: Write to <basic-memory data dir>/basic-memory.log with rotation (honors BASIC_MEMORY_CONFIG_DIR)

Fix this →

2. Stale docstring — ignore_utils.py:183

# Current (stale)
1. ~/.basic-memory/.bmignore (user's global ignore patterns)

# Should be
1. <basic-memory data dir>/.bmignore (user's global ignore patterns, honors BASIC_MEMORY_CONFIG_DIR)

Fix this →

3. # pragma: no cover on now-exercised lines — project_service.py:1141–1147

if watch_status_path.exists():
    try:  # pragma: no cover            ← now exercised — remove this
        watch_status = json.loads(  # pragma: no cover   ← now exercised — remove this
            watch_status_path.read_text(encoding="utf-8")
        )
    except Exception:  # pragma: no cover   ← keep — still hard to test
        pass

test_get_system_status_reads_watch_status_from_config_dir creates the watch-status file and calls get_system_status(), so the try: / json.loads / .read_text lines are exercised. Remove # pragma: no cover from those two lines only; leave it on except Exception:.

Fix this →

Note: I attempted to push these three fixes directly but lacked write access to the branch (403). The changes are minimal and mechanical — any of the "Fix this →" links above will apply them via Claude Code.

Overall: The five functional changes are correct and well-tested. The latest commit correctly resolves the Windows test-compatibility issue. Three minor cleanup items remain before merge.

Claude finished @groksrc's task in 58s —— View job

PR Review: fix(core): honor BASIC_MEMORY_CONFIG_DIR across remaining call sites

• Gather context
• Read files with outstanding findings
• Check previous findings status
• Post final review

What changed in the latest commit (9d07d24)

The new commit directly addresses all three findings carried across the previous two review rounds:

Code Quality Checklist

Summary

All previous findings resolved. The PR is clean:

• Five hardcoded Path.home() / ".basic-memory" call sites now correctly route through resolve_data_dir() or the threaded config object
• Five regression tests (all named *_honors_basic_memory_config_dir) guard each fix
• Deferred import in setup_logging correctly handles the config.py → utils.py circular-import constraint
• Two docstrings updated to reflect the new behavior
• # pragma: no cover cleaned up to match actual coverage

LGTM — ready to merge.