diff --git a/src/basic_memory/cli/commands/project.py b/src/basic_memory/cli/commands/project.py index 698e1b6eb..3d95c5fa4 100644 --- a/src/basic_memory/cli/commands/project.py +++ b/src/basic_memory/cli/commands/project.py @@ -805,15 +805,16 @@ def _abort_after_project_created( step: str, remedy: str, error: Exception, + retry_add_after_repair: bool = False, ) -> NoReturn: """Report a failure that happened after the project row became durable. - Two things are always true here and stated once: the project was created, and - re-running `bm project add` will refuse with "already exists". What must vary - is the remedy — it has to name a command that both fits the project's mode and - can actually repair the step that failed. A single fixed remedy pointed cloud - projects at `bm project index`, which the local-only reindex path rejects - outright (#1440 review). + The default contract states that the project was created and a blind re-run + will refuse with "already exists". When ``retry_add_after_repair`` is true, + the failed step prevented local registration, so repairing it makes re-running + the command safe: the existing adoption path finishes registration instead of + creating a second project. In either case, the remedy must fit the project's + mode and repair the step that actually failed (#1440 review). The project is deliberately not rolled back: the row is the user's, and deleting it to tidy up an error message would discard what they asked for. @@ -823,7 +824,10 @@ def _abort_after_project_created( # missing. Anything else still needs its message shown. detail = "" if isinstance(error, typer.Exit) else f": {error}" console.print(f"[yellow]Project '{name}' was created, but {step} failed{detail}[/yellow]") - console.print(f"Do not re-run 'bm project add' — '{name}' already exists. {remedy}") + if retry_add_after_repair: + console.print(f"Fix the failed step before re-running 'bm project add'. {remedy}") + else: + console.print(f"Do not re-run 'bm project add' — '{name}' already exists. {remedy}") raise typer.Exit(1) @@ -1016,6 +1020,27 @@ async def _add_project(): error=e, ) else: + if local_sync_path: + try: + Path(local_sync_path).mkdir(parents=True, exist_ok=True) + except Exception as e: + # The routing entry must not name a path that failed validation: + # config loading creates every absolute project path, so saving + # this value would make every later CLI command fail (#1441). + _abort_after_project_created( + name, + step=f"creating the local sync directory {local_sync_path}", + remedy=( + f"Create it yourself (or fix its permissions), then run this same " + f"command again: " + f"[green]{_add_command_hint(name, local_sync_path, resolved_workspace_id)}[/green]. " + f"It will adopt the project that already exists rather than create a " + f"second one." + ), + error=e, + retry_add_after_repair=True, + ) + # Trigger: local config needs enough metadata to route future commands back to cloud. # Why: explicit workspace selection and local sync state should persist across CLI sessions. # Outcome: cloud-backed projects keep cloud mode, workspace_id, and optional local sync path. @@ -1065,28 +1090,6 @@ async def _add_project(): # Save local sync path to config if in cloud mode if local_sync_path: - try: - # Create local directory if it doesn't exist - local_dir = Path(local_sync_path) - local_dir.mkdir(parents=True, exist_ok=True) - except Exception as e: - # Cloud project and routing are both saved; only the local folder - # is missing, so recovery is filesystem-side then a resync. - # `bisync` is Personal-workspace-only -- `_require_personal_workspace` - # rejects it outright for a Team workspace, and the workspace type is - # not knowable here. `pull`/`push` are additive and Team-safe, and - # work on Personal too, so they are the remedy for both (#1440 - # review). Same wording `bm cloud sync-setup` already prints. - _abort_after_project_created( - name, - step=f"creating the local sync directory {local_sync_path}", - remedy=( - f"Create it yourself (or fix its permissions), then fetch with " - f"[green]{command_hint('bm', 'cloud', 'pull', '--name', name)}[/green]." - ), - error=e, - ) - console.print(f"\n[green]Local sync path configured: {local_sync_path}[/green]") # Lead with the Team-safe additive commands (they work on any # workspace); the bisync mirror is Personal-only, so it is an aside diff --git a/tests/cli/test_project_add_post_creation_failure.py b/tests/cli/test_project_add_post_creation_failure.py index d068a1b26..6ec771ad2 100644 --- a/tests/cli/test_project_add_post_creation_failure.py +++ b/tests/cli/test_project_add_post_creation_failure.py @@ -325,8 +325,8 @@ def _run(coro: Any) -> Any: assert resolved == [] -def test_a_failed_local_sync_mkdir_gets_a_sync_remedy(tmp_path, monkeypatch, cloud_add): - """A directory that cannot be created is repaired on the filesystem, then resynced.""" +def test_a_failed_local_sync_mkdir_gets_an_add_retry_remedy(tmp_path, monkeypatch, cloud_add): + """A directory failure is repaired before add adopts and configures the project.""" # A real mkdir failure: the parent is a file, so creating a child raises. blocker = tmp_path / "blocker" blocker.write_text("not a directory") @@ -342,12 +342,12 @@ def test_a_failed_local_sync_mkdir_gets_a_sync_remedy(tmp_path, monkeypatch, clo assert "added successfully" in flat(result.output) assert "was created, but creating the local sync directory" in flat(result.output) assert "bm project index" not in flat(result.output) - # `bisync` is Personal-only; `pull` works on Team workspaces too, so it is - # the remedy for both. The full mode x step matrix lives in - # tests/cli/test_project_add_remedy_matrix.py. - assert "bm cloud pull --name research" in flat(result.output) + remedy = _printed_remedy_command(result.output) + assert remedy[:3] == ["project", "add", "research"] + assert "--local-path" in remedy + assert "bm cloud pull" not in flat(result.output) assert "bm cloud bisync" not in flat(result.output) - assert "Do not re-run 'bm project add'" in flat(result.output) + assert "Fix the failed step before re-running 'bm project add'" in flat(result.output) def test_the_remedy_survives_a_project_name_with_a_space(tmp_path, monkeypatch, cloud_add): diff --git a/tests/cli/test_project_add_remedy_matrix.py b/tests/cli/test_project_add_remedy_matrix.py index 5026a2feb..3d3e9fec4 100644 --- a/tests/cli/test_project_add_remedy_matrix.py +++ b/tests/cli/test_project_add_remedy_matrix.py @@ -353,7 +353,10 @@ def test_the_printed_remedy_actually_works( assert result.exit_code == 1 assert f"but {step}" in flat(result.output) - assert "Do not re-run 'bm project add'" in flat(result.output) + if step == SYNC_DIR: + assert "Fix the failed step before re-running 'bm project add'" in flat(result.output) + else: + assert "Do not re-run 'bm project add'" in flat(result.output) remedy = _printed_remedy_command(result.output) diff --git a/tests/cli/test_project_add_with_local_path.py b/tests/cli/test_project_add_with_local_path.py index fae4bff84..e9d42adce 100644 --- a/tests/cli/test_project_add_with_local_path.py +++ b/tests/cli/test_project_add_with_local_path.py @@ -200,6 +200,33 @@ def test_project_add_local_path_creates_nested_directories( assert nested_path.is_dir() +def test_project_add_invalid_local_path_keeps_config_loadable( + runner, mock_config, mock_api_client, tmp_path +): + """A mkdir failure must not persist a path that bricks later CLI commands.""" + blocker = tmp_path / "blocker" + blocker.write_text("not a directory") + invalid_path = blocker / "sync" + + result = runner.invoke( + app, + ["project", "add", "test-project", "--cloud", "--local-path", str(invalid_path)], + ) + + assert result.exit_code == 1 + config_data = json.loads(mock_config.read_text()) + assert "test-project" not in config_data["projects"] + + from basic_memory import config as config_module + + config_module._CONFIG_CACHE = None + config_module._CONFIG_MTIME = None + config_module._CONFIG_SIZE = None + + follow_up = runner.invoke(app, ["config", "list"]) + assert follow_up.exit_code == 0, follow_up.stdout + + def test_project_add_cloud_visibility_passes_payload(runner, mock_config, mock_api_client): """Cloud project creation should forward visibility to the API payload.""" result = runner.invoke(