feat(cli): refuse db reset while basic-memory mcp processes run

[phernandez]

Summary

• Add psutil-backed pre-flight scan to bm reset that detects live basic-memory mcp processes and refuses to proceed when any are found, surfacing PIDs + platform-appropriate cleanup commands.
• Add --force escape hatch for scripted/CI workflows that have already verified no MCP clients are attached.
• Closes Stale FTS index entries persist after reset --reindex and reindex --search #765.

Why

Reporter on #765 hit phantom search results after bm reset --reindex. Diagnosis (verified by reporter): on POSIX, Path.unlink() removes the directory entry but the inode survives as long as any process holds the file open. A live basic-memory mcp (Claude Desktop / VS Code / etc.) keeps reading the now-invisible memory.db inode and returns stale rows. Windows naturally raises PermissionError on unlink(), so this is a POSIX-only silent failure.

Per discussion, we don't auto-kill anyone else's processes — we just detect, message, and exit non-zero.

Test plan

• uv run pytest tests/cli/test_db_reset_zombie_check.py tests/cli/test_db_reset_exit.py --no-cov (11 passed)
• uv run ruff check + uv run ty check clean on changed files
• CI green

[github-actions]

Claude finished @phernandez's task in 1m 49s —— View job

---

Code Review

• Gather context and read changed files
• Review db.py implementation
• Review test files
• Check against team checklist
• Post final review

---

Solid, well-reasoned fix. The core POSIX inode problem is correctly diagnosed and the approach (detect + refuse rather than auto-kill) is the right call. A few items worth addressing before merge:

Issues

1. Process matching can false-positive on path substrings (db.py:57)

if "basic-memory" in joined and "mcp" in cmdline:

"basic-memory" in joined checks the full joined string, so a process with an argv element like /home/user/basic-memory-backup/mcp would match. The intent is to find processes where basic-memory is an argument (the entrypoint name), not a substring of an unrelated path. Consider checking list membership instead:

if "basic-memory" in cmdline and "mcp" in cmdline:

This is also more consistent — "mcp" in cmdline already does exact list membership. The test at line 45 (_FakeProc(pid=202, cmdline=["bm", "mcp"])) specifically documents that non-basic-memory entrypoints should not match, so tightening the check is in line with intent.

Fix this →

2. zombies is a misleading variable name (db.py:71, db.py:82)

In Unix parlance a "zombie" process is one that has exited but whose exit code hasn't been reaped. These are the opposite — they're very much alive. live_mcp_procs or running_mcp_procs avoids the confusion.

3. Missing unit test for --force bypass

There's no test that verifies --force=True actually suppresses the _abort_if_mcp_processes_alive() call when processes are detected. The subprocess tests in test_db_reset_exit.py use --force to pass cleanly, but they don't assert that a detected MCP process would be skipped under --force. The reset() function has # pragma: no cover so this gap won't show in coverage, but it's the key behavioral guarantee of the feature.

Team Checklist

Item	Status
Follows coding conventions (CLAUDE.md)	✅
Python 3.12+ type annotations	✅ — list[tuple[int, str]], str | None, etc.
100-character line length	✅
No commented-out code	✅
Unit tests for new functions	✅ — TestFindLiveMcpProcesses, TestAbortIfMcpProcessesAlive
Test coverage for edge cases	✅ — NoSuchProcess race, empty cmdline, self-exclusion
100% test coverage maintained	✅ — helpers tested directly; reset() body has pre-existing pragma: no cover
Windows compatibility	✅ — branched on os.name == "nt", no POSIX-only path operations
No emojis / unicode in console output	✅
No hardcoded secrets	✅
Proper error handling	✅ — NoSuchProcess / AccessDenied swallowed with continue
psutil dependency pinned sensibly	✅ — >=5.9.0 is stable and widely available
Section/decision-point comments (literate style)	✅ — thorough Why this exists docstring in _find_live_mcp_processes
Process matching false-positive risk	⚠️ — see issue #1 above
zombies naming	⚠️ — misleading Unix term, see issue #2
--force bypass tested	⚠️ — no dedicated unit test, see issue #3

Minor Observations

• The _abort_if_mcp_processes_alive docstring also says "zombie" — same renaming would apply there.
• test_exits_with_pids_when_processes_alive (line 117) uses capsys to capture Rich console output. This works because the module-level console = Console() resolves sys.stdout at write time rather than at construction time. The pattern matches existing tests in the repo so it's fine, but it's worth noting that Rich Console with an explicit file= arg would break capsys capture.

Overall the implementation is clean and the rationale is well-documented. Issues #1 and #2 are straightforward fixes; issue #3 is low-risk given the subprocess test coverage but worth adding if you want the contract explicitly tested.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4bb43d2cd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Detect bm mcp processes before allowing reset

The new pre-flight only matches processes whose command line contains the literal basic-memory, so an MCP server started via the supported bm mcp entrypoint is not detected. In that scenario bm reset still proceeds on POSIX while the live MCP process keeps the old inode open, reproducing the phantom-results problem this change is meant to block. The matcher should include the bm alias (or detect by subcommand structure) to avoid this false negative.

Useful? React with 👍 / 👎.

[github-actions]

Claude finished @phernandez's task in 1m 49s —— View job

---

Code Review (Updated — post 9c4b1f7)

• Gather context on new commit
• Read current state of db.py
• Read current state of test files
• Check if previous review issues were addressed
• Post final review

---

What the new commit does

9c4b1f7 ("test(cli): widen _patch_iter procs type for mixed fake/raising fixtures") only changes _patch_iter in test_db_reset_zombie_check.py: the procs parameter was previously typed too narrowly to accept the _Raising stub (which is a different class than _FakeProc), so the type annotation was dropped and a docstring added to explain why. This is a test-only, type-checking fix — no logic changed.

Status of prior review issues

The three issues flagged in the previous review remain unaddressed in the current state of the branch.

---

Issue #1: Substring match still present (db.py:57) — still open

if "basic-memory" in joined and "mcp" in cmdline:

"basic-memory" in joined checks the joined string for a substring, so a process with an argument like /home/alice/basic-memory-backup/start-mcp would be a false positive. "mcp" in cmdline already does exact list membership — the joined check is inconsistent and risky. The fix remains a one-liner:

if "basic-memory" in cmdline and "mcp" in cmdline:

Fix this →

---

Issue #2: zombies variable naming (db.py:71, 72, 82) — still open

In Unix parlance a zombie is a dead process whose exit code hasn't been reaped — the opposite of these very-much-alive processes. live_mcp_procs or running_mcp_procs avoids the confusion. The docstring on _abort_if_mcp_processes_alive also uses "zombie" on the same line.

Fix this →

---

Issue #3: Missing unit test for --force bypass — still open

There is no test that monkeypatches _find_live_mcp_processes to return a non-empty list and then asserts that reset(force=True, ...) does not call _abort_if_mcp_processes_alive. The subprocess tests pass --force to avoid the check during exit-semantics testing, but they don't assert the bypass contract. Since reset() is marked # pragma: no cover, this gap won't surface in coverage.

---

Codex note: bm mcp alias not detected (db.py:57)

The Codex reviewer flagged that bm mcp (the supported short alias) is intentionally excluded — test_matches_basic_memory_mcp_invocations documents pid=202, cmdline=["bm", "mcp"] as "alt entrypoint, no match". This is a deliberate design decision. Worth a short comment at the call-site acknowledging the trade-off, since users encountering phantom results via bm mcp will be puzzled that the pre-flight missed them. If the decision is to keep this behaviour as-is that's fine, but it should be explicit.

---

Team Checklist (updated)

Item	Status
Follows coding conventions (CLAUDE.md)	✅
Python 3.12+ type annotations	✅
100-character line length	✅
No commented-out code	✅
Unit tests for new functions	✅
Test coverage for edge cases	✅
100% test coverage maintained	✅
Windows compatibility	✅
No emojis / unicode in console output	✅
No hardcoded secrets	✅
Proper error handling	✅
psutil dependency pinned sensibly	✅
Literate-style comments	✅
Process matching false-positive risk (in joined)	⚠️ Issue #1 — open
zombies variable naming	⚠️ Issue #2 — open
--force bypass unit test	⚠️ Issue #3 — open
bm mcp alias detection	⚠️ Intentional exclusion, consider adding a comment

The new commit is clean and the type-annotation fix is correct. The three substantive issues from the prior round still need attention before merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9c4b1f7142

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Restrict MCP reset guard to processes using same database

This guard aborts bm reset whenever any basic-memory ... mcp process exists, but it never checks whether that process is attached to the same memory.db path being reset. In multi-profile setups (for example, one MCP server running under a different BASIC_MEMORY_HOME while resetting an isolated test/home config), bm reset now fails with exit code 1 even though no client is using the target DB, forcing users into --force for normal workflows.

Useful? React with 👍 / 👎.

[phernandez]

Codex review on 9c4b1f71:

P1 — bm mcp not detected: Real false negative, fixed in 29ebfd7c. Extracted matching to _is_basic_memory_mcp() which now recognizes basic-memory, basic_memory (Python module form), and bm/bm.exe script invocations via PurePosixPath/PureWindowsPath (so a Windows-style bm.exe path matches even when the test runs on POSIX). New test cases cover the alias entrypoint forms and a couple of negative cases (mcp as substring, unrelated server with mcp in argv).

P2 — multi-profile / different BASIC_MEMORY_HOME: Real but rare. Doing it properly means walking each detected MCP's proc.open_files() and matching against the target memory.db path, which adds a permissions/race surface (you typically can't introspect another user's process) and a chunk of logic for an advanced workflow. The --force flag is the documented escape hatch and covers the case clearly. Punting unless it shows up in real reports.

[github-actions]

Claude finished @phernandez's task in 1m 52s —— View job

---

Code Review (Updated — post 29ebfd7)

• Gather context on new commit
• Read current state of db.py
• Read current state of test files
• Check if previous review issues were addressed
• Post final review

---

What the new commit does

29ebfd7 ("fix(cli): also detect bm/bm.exe entrypoints in mcp pre-flight scan") replaces the flat "basic-memory" in joined substring check with a dedicated _is_basic_memory_mcp(cmdline) helper. The helper gates on "mcp" not in cmdline first (exact token), then iterates individual argv elements looking for basic-memory/basic_memory substrings or a stem of bm/bm.exe (via dual PurePosixPath/PureWindowsPath to handle cross-OS tests). This addresses the Codex P1 false-negative (bm mcp alias) and also makes the match logic considerably more structured.

---

Status of prior review issues

Issue #1: Substring false-positive risk — now substantially improved ✅

The old "basic-memory" in joined checked the full joined command string; the new per-arg "basic-memory" in arg is much narrower. A path like /home/user/basic-memory-backup/launch with a separate mcp arg is still technically a false positive, but that's a vanishingly unlikely real-world argv. The refactoring into _is_basic_memory_mcp is the right structural move and I'm satisfied with this.

---

Issue #2: zombies variable naming — still open ⚠️

zombies survives at db.py:98, db.py:99, and db.py:109:

zombies = _find_live_mcp_processes()   # line 98
if not zombies:                        # line 99
    ...
for pid, cmd in zombies:               # line 109

And the _find_live_mcp_processes docstring (line 72) compounds the confusion:

"transient permission error to mask a real zombie"

A zombie is a dead-but-unreaped process — the opposite of what we're detecting. live_mcp_procs or running_mcp_procs would be unambiguous. This is a one-liner rename.

Fix this →

---

Issue #3: Missing unit test for --force bypass — still open ⚠️

There is still no test that:

1. Monkeypatches _find_live_mcp_processes to return a non-empty list, and
2. Calls reset(force=True, ...) and asserts _abort_if_mcp_processes_alive is not invoked.

The subprocess tests in test_db_reset_exit.py pass --force to avoid the check during exit-semantics testing, but they don't assert the bypass contract. Since reset() is # pragma: no cover, this gap will not surface in coverage. The --force bypass is the key behavioural guarantee of the feature, particularly for the Codex P2 multi-profile scenario that was explicitly punted to --force.

Fix this →

---

New observations on _is_basic_memory_mcp

bm stem matching may over-match — PurePosixPath(arg).stem == "bm" matches any script named bm that happens to have mcp as another argv token (e.g. a bm build tool with a mcp subcommand). In practice this is extremely unlikely, but worth a short comment acknowledging the trade-off, particularly since the test at line 707 (["python", "/some/other/server.py", "mcp"]) only guards against an unrelated server — not a different bm-named tool. A comment like # 'bm' is the official alias from pyproject.toml; false-positive risk is low would make the intent explicit.

Dual-path stem check logic is correct and clever — using both PurePosixPath and PureWindowsPath to handle cross-OS test scenarios (line 51) is the right call and well-commented.

---

Team Checklist (updated)

Item	Status
Follows coding conventions (CLAUDE.md)	✅
Python 3.12+ type annotations	✅
100-character line length	✅
No commented-out code	✅
Unit tests for new functions	✅
Test coverage for edge cases	✅ — NoSuchProcess race, empty cmdline, self-exclusion, bm alias, bm.exe, Windows path
100% test coverage maintained	✅
Windows compatibility	✅
No emojis / unicode in console output	✅
No hardcoded secrets	✅
Proper error handling	✅
psutil dependency pinned sensibly	✅
Literate-style comments	✅
Process matching false-positive risk	✅ — substantially improved with _is_basic_memory_mcp
bm mcp alias detection (Codex P1)	✅ — fixed in 29ebfd7
Multi-profile / different BASIC_MEMORY_HOME (Codex P2)	ℹ️ — explicitly punted; --force is documented escape hatch
zombies variable naming	⚠️ Issue #2 — still open
--force bypass unit test	⚠️ Issue #3 — still open

The _is_basic_memory_mcp refactor is well-structured and a clear improvement. Two items (naming and a missing test) remain before merge.