fix(cli): cleanup local DB state on set-cloud/set-local

[phernandez]

Summary

Per @groksrc's direction in #680, set-cloud is now a one-way cutover instead of a routing toggle.

Before this change, set-cloud only flipped mode = cloud in config and left the project's row in the local index DB alone — so _merge_projects continued to see the project on both sides and reported source: "local+cloud" with the old local_path still populated. Confusing state, no clean way out without manually editing both config.json and memory.db.

Changes

• set-cloud now drops the project's row from the local index DB and clears path in config. On-disk note files are preserved — the user can keep, archive, or delete them as they like.
• set-local is the symmetric inverse. It accepts --local-path and recreates the local DB row at the resolved path. If the prior path is still in config (e.g. running set-local on a project that's already local), --local-path may be omitted and the existing path is reused.
• Both helpers (_detach_local_project_row, _attach_local_project_row) are idempotent: running set-cloud twice doesn't error; running set-local twice updates the path if it changed and otherwise no-ops.

Notes / behavior changes

• Breaking-ish: previously set-local <name> worked on a set-cloud'd project even though that left the DB in a hybrid state. After this change, that command path errors out and asks for --local-path. This is intentional — the prior state was the bug.
• The CLI --local/--cloud routing flags are unrelated to this; they remain a per-invocation override of routing, while set-cloud/set-local change the project's configured state.
• On-disk note files are explicitly NOT touched by either command.

Test plan

• test_set_cloud_clears_path — verifies config path is blanked after set-cloud
• test_set_local_requires_path_after_set_cloud — regression for the exact scenario in the issue: re-binding to local must not silently default
• test_set_local_success — full round trip with an explicit path
• Existing test_set_local_clears_workspace_id updated to pass --local-path
• uv run pytest tests/cli/ --no-cov — 309 passed

Closes #680

🤖 Generated with Claude Code

[github-actions]

Claude finished @phernandez's task in 2m 37s —— View job

---

Code Review: fix(cli): cleanup local DB state on set-cloud/set-local

• Gather context and read changed files
• Review code quality & standards
• Review testing & documentation
• Review architecture & compatibility
• Post final review

---

The implementation is clean and addresses the root cause correctly. A few issues worth fixing before merge:

---

Issues

1. Missing type annotations on helper functions (violates CLAUDE.md convention)

Both _detach_local_project_row and _attach_local_project_row omit a type annotation for app_config. CLAUDE.md requires full type annotations.

project.py line 841:

async def _detach_local_project_row(app_config, name: str) -> bool:
#                          ↑ no annotation

Should be:

async def _detach_local_project_row(app_config: BasicMemoryConfig, name: str) -> bool:

Same for _attach_local_project_row at line 869. Fix this →

---

2. Windows path compatibility issue in test assertion

test_project_set_cloud_local.py line 175:

assert config_data["projects"]["research"]["path"] == str(new_path)

resolved_path in set_local is normalized via .as_posix() (forward slashes), but str(new_path) on Windows produces backslashes. This test would fail on Windows. It should be:

assert config_data["projects"]["research"]["path"] == Path(new_path).as_posix()

Same issue on line 229 in test_set_local_clears_workspace_id. Fix this →

---

3. Coverage gaps in the two helper functions

The mock_config fixture sets up a config JSON but no database. Every test that invokes _detach_local_project_row hits a fresh empty DB, so repo.get_by_name(name) always returns None. This means:

• _detach_local_project_row: the return True branch (actual row deletion) is never exercised
• _attach_local_project_row: the repo.update_path(existing.id, path) branch (update stale path) is never exercised

Since CLAUDE.md mandates 100% coverage, these branches need tests. Two new test scenarios would cover them:

1. Pre-populate a row in the DB before calling set-cloud → verify detached=True message appears in output
2. Call set-local twice with different paths → verify the path is updated on the second call

---

4. shutdown_db() tears down global DB state

Both helpers call await db.shutdown_db() in their finally blocks. shutdown_db() disposes the global _engine and resets _engine/_session_maker to None. This is safe for standalone CLI invocations, but if these helpers are ever called from within the running MCP server or API server (which share the same process-level DB singleton), it would silently destroy the shared DB connection.

This is currently only reachable from CLI commands, so it's acceptable — but worth a comment making the assumption explicit:

finally:
    # CLI-only: safe to tear down the global DB singleton here since
    # set-cloud/set-local are never called from within the long-running server.
    await db.shutdown_db()

---

Checklist

Item	Status
Follows CLAUDE.md coding conventions	⚠️ Missing type annotations on helpers
Python 3.12+ type annotations	⚠️ app_config untyped
SQLAlchemy 2.0 async patterns	✅
FastAPI / Typer conventions	✅
100-character line limit	✅
No commented-out code	✅
Unit tests for new functions	⚠️ Coverage gaps in both helpers (see issue #3)
100% test coverage maintained	⚠️ return True and update_path branches untested
Literate programming / Trigger-Why-Outcome comments	✅ Excellent
DB changes / Alembic migrations	✅ N/A (no schema changes)
Local-first architecture preserved	✅
Knowledge graph consistency	✅
On-disk file integrity	✅ Explicitly not touched
No hardcoded secrets	✅
Input validation	✅
Error handling	✅ Idempotent, fail-fast
Windows path compatibility	⚠️ Test assertions use str(path) instead of Path.as_posix()
No emojis/unicode in console output	✅

---

Summary: Solid fix for the confusing local+cloud merge state. The logic is correct and the docstrings are well-structured. The three actionable issues are: add BasicMemoryConfig type annotations to the two helpers, fix Windows-incompatible path assertions in two test cases, and add tests for the uncovered branches (return True in detach, update_path in attach).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a42c6a2a28

ℹ️ 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]

Reject relative saved paths when falling back in set-local

When --local-path is omitted, this command reuses entry.path and immediately runs abspath() on it. Legacy cloud entries can carry non-absolute placeholder paths (the config migration comments explicitly call out slug paths), so this silently rebases them to the current working directory and writes that to config/DB. In that scenario set-local can attach the project to an unintended directory without user confirmation; require an absolute fallback path (or force --local-path) before resolving.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Persist config before deleting the local project row

The command deletes the local DB row before writing the updated config. If save_config fails (e.g., disk full/permissions), the project remains local in config but its DB row and indexed state are already removed, leaving the local project in a broken hybrid state until manual repair/reindex. This should be made atomic (or rollback the delete on save failure) so partial failure doesn’t corrupt local state.

Useful? React with 👍 / 👎.

[github-actions]

Claude finished @phernandez's task in 4m 2s —— View job

---

Code Review: fix(cli): cleanup local DB state on set-cloud/set-local

• Gather context and read changed files
• Review code quality & standards
• Review testing & documentation
• Review architecture & compatibility
• Implement fixes (push blocked by repo permissions — see diff below)

---

Summary

The Windows path fix from the previous review (str(new_path) → new_path.as_posix()) is already in on commit 28c49b6. Three issues remain; I've prepared a patch for all three.

---

Issues Found

1. Missing type annotations (CLAUDE.md violation)

app_config is untyped in both helper functions.

project.py line 841:

# Before
async def _detach_local_project_row(app_config, name: str) -> bool:
# After
async def _detach_local_project_row(app_config: BasicMemoryConfig, name: str) -> bool:

Same for _attach_local_project_row at line 869. Requires adding BasicMemoryConfig to the import at line 29.

---

2. Coverage gaps — two helper branches never exercised

The mock_config fixture creates no database rows, so:

• _detach_local_project_row: the repo.delete() → return True branch is never hit
• _attach_local_project_row: the repo.update_path() branch is never hit (row always starts absent)

New tests needed:

# In TestSetCloudCutover — covers repo.delete() return True
def test_set_cloud_removes_existing_db_row(self, runner, mock_config, tmp_path):
    research_path = tmp_path / "research"
    runner.invoke(app, ["project", "set-local", "research", "--local-path", str(research_path)])
    result = runner.invoke(app, ["project", "set-cloud", "research"])
    assert result.exit_code == 0
    assert "local index entry removed" in result.stdout.lower()

# In TestSetLocal — covers repo.update_path()
def test_set_local_update_path_when_row_exists(self, runner, mock_config, tmp_path):
    path_a = tmp_path / "research_v1"
    path_b = tmp_path / "research_v2"
    runner.invoke(app, ["project", "set-local", "research", "--local-path", str(path_a)])
    result = runner.invoke(app, ["project", "set-local", "research", "--local-path", str(path_b)])
    assert result.exit_code == 0
    assert json.loads(mock_config.read_text())["projects"]["research"]["path"] == path_b.as_posix()

---

3. shutdown_db() CLI-only assumption undocumented

Both finally blocks dispose the global _engine. This is safe only when called from a CLI process that doesn't share the singleton with an MCP/API server. Worth noting explicitly:

finally:
    # CLI-only: safe to tear down the global DB singleton here since
    # set-cloud/set-local are never called from within the long-running server.
    await db.shutdown_db()

---

Prepared Patch

I prepared a commit (1e646a1) addressing all three issues but the push was blocked by repository permissions. The diff:

src/basic_memory/cli/commands/project.py

-from basic_memory.config import ConfigManager, ProjectEntry, ProjectMode
+from basic_memory.config import BasicMemoryConfig, ConfigManager, ProjectEntry, ProjectMode

-async def _detach_local_project_row(app_config, name: str) -> bool:
+async def _detach_local_project_row(app_config: BasicMemoryConfig, name: str) -> bool:
     ...
     finally:
+        # CLI-only: safe to tear down the global DB singleton here since
+        # set-cloud/set-local are never called from within the long-running server.
         await db.shutdown_db()

-async def _attach_local_project_row(app_config, name: str, path: str) -> None:
+async def _attach_local_project_row(app_config: BasicMemoryConfig, name: str, path: str) -> None:
     ...
     finally:
+        # CLI-only: safe to tear down the global DB singleton here since
+        # set-cloud/set-local are never called from within the long-running server.
         await db.shutdown_db()

Plus the two new tests shown above.

---

Checklist

Item	Status
Follows CLAUDE.md coding conventions	⚠️ Missing type annotations (patch ready)
Python 3.12+ type annotations	⚠️ app_config untyped (patch ready)
SQLAlchemy 2.0 async patterns	✅
FastAPI / Typer conventions	✅
100-character line length limit	✅
No commented-out code	✅
Unit tests for new functions	⚠️ Two helper branches uncovered (patch ready)
100% test coverage maintained	⚠️ return True and update_path branches untested (patch ready)
Literate programming / Trigger-Why-Outcome comments	✅ Excellent
DB changes / Alembic migrations	✅ N/A
Local-first architecture preserved	✅
Windows path compatibility	✅ Fixed in 28c49b6
No emojis/unicode in console output	✅
shutdown_db() assumption documented	⚠️ Needs comment (patch ready)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 28c49b6aa4

ℹ️ 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]

Save config before reattaching local project row

set-local recreates/updates the local DB project row before persisting the config change. If save_config fails (for example due to disk/permission errors), config still says the project is cloud-routed while the local DB row has already been reintroduced, recreating the same hybrid local+cloud state this change is trying to eliminate and requiring manual cleanup. Make this transition atomic (or rollback the DB write on save failure) so partial failures do not corrupt project state.

Useful? React with 👍 / 👎.

[github-actions]

Claude finished @phernandez's task in 2m 36s —— View job

---

Code Review: fix(cli): cleanup local DB state on set-cloud/set-local

• Gather context and read changed files
• Review code quality & standards
• Review testing & documentation
• Review architecture & compatibility
• Post final review

---

All three issues raised in the previous round of reviews have been addressed. This PR is ready to merge with one minor note worth a judgment call.

---

Resolved since last review ✅

Issue	Resolution
Missing BasicMemoryConfig type annotation on both helpers	Fixed in a42c6a2 — app_config: BasicMemoryConfig added, import added at line 29
Windows-incompatible str(new_path) in test assertions	Fixed in 28c49b6 — both assertions now use new_path.as_posix()
_detach_local_project_row return True branch never exercised	Fixed in 8f26f15 — test_set_cloud_removes_existing_db_row seeds a DB row then calls set-cloud, asserting the "local index entry removed" message
_attach_local_project_row update_path branch never exercised	Fixed in 8f26f15 — test_set_local_update_path_when_row_exists calls set-local twice with different paths, verifying config reflects the second path
shutdown_db() CLI-only assumption undocumented	Fixed in a42c6a2 — both finally blocks carry the CLI-only comment

---

One remaining concern (Codex P2 — judgment call)

set-local operation ordering could transiently recreate the hybrid state

project.py:1034 — _attach_local_project_row runs before config_manager.save_config():

run_with_cleanup(_attach_local_project_row(config, name, resolved_path))  # DB row created

config.set_project_mode(name, ProjectMode.LOCAL)                          # ← if save fails
config.projects[name].workspace_id = None                                 # ↑ after this,
config.projects[name].path = resolved_path                                # ↑ config still says cloud
config_manager.save_config(config)

If save_config fails (disk full, permissions) after the DB attach succeeds, config still shows mode=cloud while the local DB has a new row — exactly the local+cloud hybrid state this PR eliminates. Running set-local again with the same path recovers the situation idempotently, so it's not permanent. The analogous path in set-cloud (project.py:967) has a comment explaining the ordering was intentional; set-local could use the same. Alternatively, swapping the order (save config first, then attach DB row) would eliminate the regressed-bug window at the cost of a different broken state on failure (correct config, missing DB row — which sync would self-heal on next run).

This is a rare failure mode and both orderings are survivable. Mentioning it for visibility, not as a blocker.

---

Checklist

Item	Status
Follows CLAUDE.md coding conventions	✅
Python 3.12+ type annotations	✅ Fixed
SQLAlchemy 2.0 async patterns	✅
FastAPI / Typer conventions	✅
100-character line length limit	✅
No commented-out code	✅
Unit tests for new functions	✅
100% test coverage (all branches exercised)	✅ Fixed
Literate programming / Trigger-Why-Outcome comments	✅ Excellent
DB changes / Alembic migrations	✅ N/A
Local-first architecture preserved	✅
On-disk file integrity	✅ Explicitly not touched
Windows path compatibility	✅ Fixed
No emojis/unicode in console output	✅
shutdown_db() assumption documented	✅ Fixed
set-local operation ordering	⚠️ Low-risk edge case (see above)

---

Summary: All blocking issues from prior reviews are resolved. The implementation is clean, idempotent, and well-tested. The one open item (operation ordering in set-local) is a rare-failure edge case with a recoverable path — worth a one-line comment if you'd like to document the tradeoff, but not a merge blocker.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f26f159c7

ℹ️ 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]

Preserve local sync path during set-cloud cutover

Do not blank entry.path for every cloud cutover, because cloud projects with an existing local bisync copy rely on that path to stay eligible for local watchers/sync. After this line runs, local_sync_path may still be set but both cloud filters (services/initialization.py and sync/watch_service.py) check Path(entry.path).is_absolute() to decide whether to keep cloud projects, so rerunning set-cloud (for example to change workspace) can silently disable background sync/watch for that project’s local mirror.

Useful? React with 👍 / 👎.

[phernandez]

Quick triage of the Codex review on 8f26f159:

P1 — preserve entry.path for cloud projects with bisync mirrors: Real concern, but it conflicts with the explicit set-cloud design from #680: a one-way cutover where the project's configured state ends up purely cloud. The bisync-mirror case still works through entry.local_sync_path (preserved here), and bm cloud bisync reads from that field; only the continuous watch_service filter (Path(entry.path).is_absolute()) is affected. The supported recovery for a user who really wanted to keep the watcher is bm project set-local <name> --local-path <mirror> followed by bm project add --cloud --local-path <mirror> — same path the add --cloud flow uses to seed both fields. Leaving as-is to honor the design choice.

P2 — relative-path fallback in set-local: Not a regression introduced here. Before this PR entry.path was already passed straight through os.path.abspath(), so legacy slug paths could rebase to CWD then too. The new behavior actually makes this harder to hit because set-cloud blanks entry.path, so a subsequent set-local with no --local-path falls into the if not candidate guard. Worth tightening separately if it ever shows up in the wild.

P2 — save_config ordering on set-cloud and set-local: Both ordering choices have a partial-failure mode (DB-row-only or config-only). A real atomic transaction would need the project DB and the JSON config under one writer, which is well outside the scope of this fix. The current order — DB first, then config — keeps the user-visible state (bm project list) in sync with the actual routing target if the config save raises mid-write, which is the failure mode I'd rather have. Punting.

CI is green; merging.