From a42c6a2a288e137be65c4b7bb0de1b611643c976 Mon Sep 17 00:00:00 2001 From: phernandez Date: Wed, 29 Apr 2026 10:04:33 -0500 Subject: [PATCH 1/3] fix(cli): cleanup local DB state on set-cloud/set-local MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per maintainer direction (#680), `set-cloud` is now a one-way cutover rather than 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. set-local on a project that was 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. Tests: - `test_set_cloud_clears_path` — config `path` blanked after set-cloud - `test_set_local_requires_path_after_set_cloud` — regression for the exact loop the reporter described: re-binding to local must not silently default - `test_set_local_success` — full round trip with an explicit path Closes #680 Signed-off-by: phernandez --- src/basic_memory/cli/commands/project.py | 124 +++++++++++++++++++++- tests/cli/test_project_set_cloud_local.py | 49 +++++++-- 2 files changed, 161 insertions(+), 12 deletions(-) diff --git a/src/basic_memory/cli/commands/project.py b/src/basic_memory/cli/commands/project.py index c7db3abf..106fbf75 100644 --- a/src/basic_memory/cli/commands/project.py +++ b/src/basic_memory/cli/commands/project.py @@ -838,6 +838,72 @@ async def _move_project(): raise typer.Exit(1) +async def _detach_local_project_row(app_config, name: str) -> bool: + """Drop the project's row from the local index DB. + + Trigger: `bm project set-cloud` is making a project cloud-only. + Why: the local row is what causes `_merge_projects` to report + `source: "local+cloud"` after the toggle (#680). Removing it + forces the merged listing to honor the user's chosen mode. + Outcome: returns True if a row was deleted, False if there was + nothing to clean up. On-disk note files are not touched. + """ + from basic_memory import db + from basic_memory.repository import ProjectRepository + + _, session_maker = await db.get_or_create_db( + db_path=app_config.database_path, + db_type=db.DatabaseType.FILESYSTEM, + ) + try: + repo = ProjectRepository(session_maker) + existing = await repo.get_by_name(name) + if existing is None: + return False + await repo.delete(existing.id) + return True + finally: + await db.shutdown_db() + + +async def _attach_local_project_row(app_config, name: str, path: str) -> None: + """Ensure the project has a row in the local index DB at the given path. + + Trigger: `bm project set-local` is making a previously cloud-only + project local again. + Why: without a row in the local DB, every local-side tool (`list`, + `info`, sync, indexing) would skip this project. + Outcome: a row is created if missing, or its path is updated to match + the new local home if it already exists. On-disk files are not + touched — the caller is responsible for ensuring the directory + exists. + """ + from basic_memory import db + from basic_memory.repository import ProjectRepository + + _, session_maker = await db.get_or_create_db( + db_path=app_config.database_path, + db_type=db.DatabaseType.FILESYSTEM, + ) + try: + repo = ProjectRepository(session_maker) + existing = await repo.get_by_name(name) + if existing is None: + await repo.create( + { + "name": name, + "path": path, + "permalink": generate_permalink(name), + "is_active": True, + } + ) + return + if existing.path != path: + await repo.update_path(existing.id, path) + finally: + await db.shutdown_db() + + @project_app.command("set-cloud") def set_cloud( name: str = typer.Argument(..., help="Name of the project to route through cloud"), @@ -855,6 +921,13 @@ def set_cloud( If omitted, uses the default workspace (if set) or auto-selects when only one workspace is available. + This is a one-way cutover: the project's row in the local index DB is + removed and the local path in config is cleared so the project's + configured state is purely cloud. On-disk note files are preserved — + the caller can keep, archive, or delete them as they see fit. To + return to local mode use `bm project set-local --local-path + `. + Examples: bm project set-cloud research --workspace Personal bm project set-cloud research --workspace 11111111-... @@ -883,27 +956,52 @@ def set_cloud( resolved_workspace_id = _resolve_workspace_id(config, workspace) + # Drop the local DB row first so the user-visible state stays consistent + # even if the config save below raises for some reason. Idempotent: a + # second `set-cloud` simply finds no row and returns False. + previous_path = config.projects[name].path + detached = run_with_cleanup(_detach_local_project_row(config, name)) + config.set_project_mode(name, ProjectMode.CLOUD) if resolved_workspace_id: config.projects[name].workspace_id = resolved_workspace_id + # Clear local path: source-of-truth for this project is now the cloud + config.projects[name].path = "" config_manager.save_config(config) console.print(f"[green]Project '{name}' set to cloud mode[/green]") if resolved_workspace_id: console.print(f"[dim]Workspace: {resolved_workspace_id}[/dim]") + if detached and previous_path: + console.print( + f"[dim]Local index entry removed. Files at {previous_path} are preserved on disk.[/dim]" + ) console.print("[dim]MCP tools and CLI commands for this project will route through cloud[/dim]") @project_app.command("set-local") def set_local( name: str = typer.Argument(..., help="Name of the project to revert to local mode"), + local_path: str = typer.Option( + None, + "--local-path", + help=( + "Local filesystem path for this project. Required unless the project " + "was previously local and its prior path is still in config." + ), + ), ) -> None: """Revert a project to local mode (use in-process ASGI transport). - Clears any associated cloud workspace. + Recreates the project's row in the local index DB and clears any + associated cloud workspace. If the project was previously local and + its prior path is still in config (e.g. an older version that didn't + blank `path` on `set-cloud`), `--local-path` may be omitted and that + path will be reused. - Example: - bm project set-local research + Examples: + bm project set-local research --local-path ~/Documents/research + bm project set-local research # reuse prior path """ config_manager = ConfigManager() config = config_manager.config @@ -913,11 +1011,31 @@ def set_local( console.print(f"[red]Error: Project '{name}' not found in config[/red]") raise typer.Exit(1) + entry = config.projects[name] + candidate = local_path or entry.path + if not candidate: + console.print( + f"[red]Error: --local-path is required for '{name}' " + "(no previous local path is recorded)[/red]" + ) + raise typer.Exit(1) + + resolved_path = Path(os.path.abspath(os.path.expanduser(candidate))).as_posix() + + # Recreate the local DB row. Idempotent: if the row exists with the + # same path it's a no-op; if it exists at a stale path the path is + # updated. The directory itself is not auto-created — the user is + # expected to know whether they want to start a fresh project tree + # or point at an existing one. + run_with_cleanup(_attach_local_project_row(config, name, resolved_path)) + config.set_project_mode(name, ProjectMode.LOCAL) config.projects[name].workspace_id = None + config.projects[name].path = resolved_path config_manager.save_config(config) console.print(f"[green]Project '{name}' set to local mode[/green]") + console.print(f"[dim]Path: {resolved_path}[/dim]") console.print("[dim]MCP tools and CLI commands for this project will use local transport[/dim]") diff --git a/tests/cli/test_project_set_cloud_local.py b/tests/cli/test_project_set_cloud_local.py index e8024274..54dac743 100644 --- a/tests/cli/test_project_set_cloud_local.py +++ b/tests/cli/test_project_set_cloud_local.py @@ -154,21 +154,25 @@ def test_set_cloud_with_oauth_session(self, runner, tmp_path, monkeypatch): class TestSetLocal: """Tests for bm project set-local command.""" - def test_set_local_success(self, runner, mock_config): + def test_set_local_success(self, runner, mock_config, tmp_path): """Test reverting a project to local mode.""" - # First set to cloud + # First set to cloud (clears the local path as part of the cutover) runner.invoke(app, ["project", "set-cloud", "research"]) config_data = json.loads(mock_config.read_text()) assert config_data["projects"]["research"]["mode"] == "cloud" - # Now set back to local - result = runner.invoke(app, ["project", "set-local", "research"]) + # Now set back to local — must supply a path since set-cloud blanked it + new_path = tmp_path / "research" + result = runner.invoke( + app, ["project", "set-local", "research", "--local-path", str(new_path)] + ) assert result.exit_code == 0 assert "local mode" in result.stdout.lower() - # Verify config was updated — mode reset to local + # Verify config was updated — mode reset to local, path restored config_data = json.loads(mock_config.read_text()) assert config_data["projects"]["research"]["mode"] == "local" + assert config_data["projects"]["research"]["path"] == str(new_path) def test_set_local_nonexistent_project(self, runner, mock_config): """Test set-local with a project that doesn't exist in config.""" @@ -177,12 +181,23 @@ def test_set_local_nonexistent_project(self, runner, mock_config): assert "not found" in result.stdout.lower() def test_set_local_already_local(self, runner, mock_config): - """Test set-local on a project that's already local (no-op, should succeed).""" + """Test set-local on a project that's already local (reuses existing path).""" result = runner.invoke(app, ["project", "set-local", "main"]) assert result.exit_code == 0 assert "local mode" in result.stdout.lower() - def test_set_local_clears_workspace_id(self, runner, mock_config): + def test_set_local_requires_path_after_set_cloud(self, runner, mock_config): + """Regression for #680: after set-cloud blanks the path, set-local must + refuse to silently default — the user has to specify where the project + lives now.""" + runner.invoke(app, ["project", "set-cloud", "research"]) + + # No --local-path; config no longer has a path either. + result = runner.invoke(app, ["project", "set-local", "research"]) + assert result.exit_code == 1 + assert "--local-path" in result.stdout + + def test_set_local_clears_workspace_id(self, runner, mock_config, tmp_path): """Test that set-local clears workspace_id from the project entry.""" from basic_memory import config as config_module @@ -198,8 +213,12 @@ def test_set_local_clears_workspace_id(self, runner, mock_config): config_module._CONFIG_MTIME = None config_module._CONFIG_SIZE = None - # Set back to local - result = runner.invoke(app, ["project", "set-local", "research"]) + # Set back to local — supply --local-path; existing config path is preserved + # in this test setup, but new behavior recommends explicit path passing. + new_path = tmp_path / "research" + result = runner.invoke( + app, ["project", "set-local", "research", "--local-path", str(new_path)] + ) assert result.exit_code == 0 # Verify workspace_id was cleared @@ -211,6 +230,18 @@ def test_set_local_clears_workspace_id(self, runner, mock_config): assert updated_data["projects"]["research"]["mode"] == "local" +class TestSetCloudCutover: + """Regression tests for #680 — set-cloud as a one-way cutover.""" + + def test_set_cloud_clears_path(self, runner, mock_config): + """After set-cloud, config.projects[name].path must be blanked so the + merged project list reports source: cloud (not local+cloud).""" + runner.invoke(app, ["project", "set-cloud", "research"]) + updated = json.loads(mock_config.read_text()) + assert updated["projects"]["research"]["mode"] == "cloud" + assert updated["projects"]["research"]["path"] == "" + + class TestSetCloudWithWorkspace: """Tests for 'bm project set-cloud --workspace' option.""" From 28c49b6aa4b16aefb4b522dfd34a4d8e68bd7115 Mon Sep 17 00:00:00 2001 From: phernandez Date: Wed, 29 Apr 2026 11:02:11 -0500 Subject: [PATCH 2/3] test(cli): normalize path comparison in set-local test for Windows set-local stores `path` via Path(...).as_posix() (matching bm project add and the rest of the codebase, which keep paths as POSIX strings). The test compared against str(Path), which on Windows produces backslashes and fails the equality check on the Windows CI runner. Use .as_posix() in the assertion so the test matches what the production code actually writes to config. Signed-off-by: phernandez --- tests/cli/test_project_set_cloud_local.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/cli/test_project_set_cloud_local.py b/tests/cli/test_project_set_cloud_local.py index 54dac743..5852d7ed 100644 --- a/tests/cli/test_project_set_cloud_local.py +++ b/tests/cli/test_project_set_cloud_local.py @@ -169,10 +169,13 @@ def test_set_local_success(self, runner, mock_config, tmp_path): assert result.exit_code == 0 assert "local mode" in result.stdout.lower() - # Verify config was updated — mode reset to local, path restored + # Verify config was updated — mode reset to local, path restored. + # set-local normalizes the path via Path.as_posix(), matching the + # convention used by `bm project add`. On Windows that means + # backslashes are converted to forward slashes. config_data = json.loads(mock_config.read_text()) assert config_data["projects"]["research"]["mode"] == "local" - assert config_data["projects"]["research"]["path"] == str(new_path) + assert config_data["projects"]["research"]["path"] == new_path.as_posix() def test_set_local_nonexistent_project(self, runner, mock_config): """Test set-local with a project that doesn't exist in config.""" From 8f26f159c71a75fb45c04bf99023f085e96c183c Mon Sep 17 00:00:00 2001 From: phernandez Date: Wed, 29 Apr 2026 11:21:31 -0500 Subject: [PATCH 3/3] test(cli): cover update_path and existing-row delete branches Address reviewer follow-ups on #775: - Add type annotations to the two CLI helpers (BasicMemoryConfig). - Note the CLI-only assumption inline at each shutdown_db() call. - Add two tests that exercise the previously uncovered branches: - test_set_local_update_path_when_row_exists hits repo.update_path. - test_set_cloud_removes_existing_db_row hits repo.delete on the detach helper and asserts the user-visible cleanup message. Signed-off-by: phernandez --- src/basic_memory/cli/commands/project.py | 10 ++++-- tests/cli/test_project_set_cloud_local.py | 39 +++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/basic_memory/cli/commands/project.py b/src/basic_memory/cli/commands/project.py index 106fbf75..2bc4b411 100644 --- a/src/basic_memory/cli/commands/project.py +++ b/src/basic_memory/cli/commands/project.py @@ -26,7 +26,7 @@ ) from basic_memory.cli.commands.command_utils import get_project_info, run_with_cleanup from basic_memory.cli.commands.routing import force_routing, validate_routing_flags -from basic_memory.config import ConfigManager, ProjectEntry, ProjectMode +from basic_memory.config import BasicMemoryConfig, ConfigManager, ProjectEntry, ProjectMode from basic_memory.mcp.async_client import get_client, resolve_configured_workspace from basic_memory.mcp.clients import ProjectClient from basic_memory.schemas.cloud import ( @@ -838,7 +838,7 @@ async def _move_project(): raise typer.Exit(1) -async def _detach_local_project_row(app_config, name: str) -> bool: +async def _detach_local_project_row(app_config: BasicMemoryConfig, name: str) -> bool: """Drop the project's row from the local index DB. Trigger: `bm project set-cloud` is making a project cloud-only. @@ -863,10 +863,12 @@ async def _detach_local_project_row(app_config, name: str) -> bool: await repo.delete(existing.id) return True finally: + # CLI-only: safe to tear down the global DB singleton here since + # set-cloud/set-local never run inside a long-lived MCP/API 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: """Ensure the project has a row in the local index DB at the given path. Trigger: `bm project set-local` is making a previously cloud-only @@ -901,6 +903,8 @@ async def _attach_local_project_row(app_config, name: str, path: str) -> None: if existing.path != path: await repo.update_path(existing.id, path) finally: + # CLI-only: safe to tear down the global DB singleton here since + # set-cloud/set-local never run inside a long-lived MCP/API server. await db.shutdown_db() diff --git a/tests/cli/test_project_set_cloud_local.py b/tests/cli/test_project_set_cloud_local.py index 5852d7ed..a00cf8b6 100644 --- a/tests/cli/test_project_set_cloud_local.py +++ b/tests/cli/test_project_set_cloud_local.py @@ -232,6 +232,28 @@ def test_set_local_clears_workspace_id(self, runner, mock_config, tmp_path): assert updated_data["projects"]["research"]["workspace_id"] is None assert updated_data["projects"]["research"]["mode"] == "local" + def test_set_local_update_path_when_row_exists(self, runner, mock_config, tmp_path): + """Re-running set-local with a different --local-path must update the + existing DB row (covers the repo.update_path() branch in + _attach_local_project_row).""" + path_a = tmp_path / "research_v1" + path_b = tmp_path / "research_v2" + + # First call seeds the DB row at path_a. + result_a = runner.invoke( + app, ["project", "set-local", "research", "--local-path", str(path_a)] + ) + assert result_a.exit_code == 0 + + # Second call must update the existing row's path to path_b. + result_b = runner.invoke( + app, ["project", "set-local", "research", "--local-path", str(path_b)] + ) + assert result_b.exit_code == 0 + + updated = json.loads(mock_config.read_text()) + assert updated["projects"]["research"]["path"] == path_b.as_posix() + class TestSetCloudCutover: """Regression tests for #680 — set-cloud as a one-way cutover.""" @@ -244,6 +266,23 @@ def test_set_cloud_clears_path(self, runner, mock_config): assert updated["projects"]["research"]["mode"] == "cloud" assert updated["projects"]["research"]["path"] == "" + def test_set_cloud_removes_existing_db_row(self, runner, mock_config, tmp_path): + """When set-cloud runs against a project with a DB row, the row must + be removed and the user-facing message must mention the cleanup + (covers the repo.delete() → return True branch in + _detach_local_project_row).""" + # Seed a DB row first by going through set-local. + research_path = tmp_path / "research" + seed = runner.invoke( + app, ["project", "set-local", "research", "--local-path", str(research_path)] + ) + assert seed.exit_code == 0 + + # Now flip to cloud — _detach_local_project_row should find and drop the row. + result = runner.invoke(app, ["project", "set-cloud", "research"]) + assert result.exit_code == 0 + assert "local index entry removed" in result.stdout.lower() + class TestSetCloudWithWorkspace: """Tests for 'bm project set-cloud --workspace' option."""