Skip to content

Commit 51a529b

Browse files
groksrcclaude
andcommitted
fix(sync): also exclude orphan DB projects absent from config (#949)
Address Codex review feedback. The previous path-only filter dropped an implicit protection: get_project_mode() defaults projects missing from config to CLOUD, so the old mode-based guard skipped stale DB rows that had been removed from config. With a path-only check, an orphan row with an absolute path would pass and background sync/watch could still mutate a directory the user already removed from config (config is the source of truth) if reconciliation was skipped or failed. Introduce BasicMemoryConfig.is_locally_syncable(name, path), which requires both config membership and an absolute path, and use it from both the background sync selection and the watch cycle so the two paths cannot diverge. Add direct unit tests for the helper plus an orphan-row regression test for the watch selection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
1 parent 0414ce9 commit 51a529b

6 files changed

Lines changed: 102 additions & 21 deletions

File tree

src/basic_memory/config.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -658,6 +658,26 @@ def get_project_mode(self, project_name: str) -> ProjectMode:
658658
entry = self.projects.get(project_name)
659659
return entry.mode if entry else ProjectMode.CLOUD
660660

661+
def is_locally_syncable(self, project_name: str, project_path: str) -> bool:
662+
"""Whether a project should be synced/watched on the local filesystem.
663+
664+
Both conditions are required (issue #949):
665+
666+
* The project is present in config. Config is the source of truth, so a
667+
stale database row that was removed from config — but whose deletion
668+
has not yet been reconciled, or whose reconciliation failed — must
669+
not be synced even though it still has a real directory on disk.
670+
* Its path is absolute. An empty or relative path resolves against the
671+
process cwd, so syncing it would adopt whatever directory the server
672+
was launched from as the project root and mutate unrelated files.
673+
674+
Cloud-only projects (empty/slug path) and cloud projects with a real
675+
local bisync copy (absolute path) are handled correctly by these two
676+
conditions, so no separate mode check is needed.
677+
"""
678+
entry = self.projects.get(project_name)
679+
return entry is not None and Path(project_path).is_absolute()
680+
661681
def set_project_mode(self, project_name: str, mode: ProjectMode) -> None:
662682
"""Set the routing mode for a project.
663683

src/basic_memory/services/initialization.py

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -120,20 +120,14 @@ async def initialize_file_sync(
120120
active_projects = [p for p in active_projects if p.name == constrained_project]
121121
logger.info(f"Background sync constrained to project: {constrained_project}")
122122

123-
# Only projects with an absolute local path are safe to sync.
124-
# Trigger: a project whose path is empty or relative.
125-
# Why: Path("") and other relative paths resolve against the process cwd, so
126-
# syncing such a project would adopt whatever directory the server was
127-
# launched from as the project root and inject frontmatter into unrelated
128-
# markdown files there (issue #949). Empty paths come from cloud-only
129-
# projects, but also from any hand-edited config that left mode at the
130-
# LOCAL default, so the gate is on the path itself, not the project mode.
131-
# Outcome: such projects are excluded from local sync. Cloud projects that
132-
# have a real local bisync copy keep their absolute path and still sync.
133-
skip = [p.name for p in active_projects if not Path(p.path).is_absolute()]
123+
# Only sync projects that are in config (source of truth) and have an
124+
# absolute local path; see BasicMemoryConfig.is_locally_syncable. This keeps
125+
# background sync from adopting the process cwd as a project root and
126+
# mutating unrelated files (issue #949).
127+
skip = [p.name for p in active_projects if not app_config.is_locally_syncable(p.name, p.path)]
134128
if skip:
135129
active_projects = [p for p in active_projects if p.name not in skip]
136-
logger.info(f"Skipping projects without an absolute local path for sync: {skip}")
130+
logger.info(f"Skipping projects that are not locally syncable for sync: {skip}")
137131

138132
# Start sync for all projects as background tasks (non-blocking)
139133
async def sync_project_background(project: Project):

src/basic_memory/sync/watch_service.py

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -184,22 +184,24 @@ async def _select_projects_to_watch(self) -> list[Project]:
184184
``--project``, only that project is watched. This keeps concurrent
185185
MCP processes from producing duplicate watchers that race on the
186186
same files.
187-
2. Projects without an absolute local path are skipped. A non-absolute
188-
path (empty string for cloud-only projects, or any relative value)
189-
resolves against the process cwd, so watching it would observe and
190-
mutate whatever directory the server was launched from (issue #949).
191-
Cloud projects with a local bisync copy keep their absolute path and
192-
are still watched.
187+
2. Projects that are not locally syncable are skipped — those missing
188+
from config (config is the source of truth, so stale DB rows must
189+
not be watched) or with a non-absolute path (which would resolve
190+
against the process cwd and make the watcher observe and mutate the
191+
directory the server was launched from). See
192+
``BasicMemoryConfig.is_locally_syncable`` (issue #949). Cloud
193+
projects with a local bisync copy keep their absolute path and are
194+
still watched.
193195
"""
194196
projects = await self.project_repository.get_active_projects()
195197

196198
if self.constrained_project:
197199
projects = [p for p in projects if p.name == self.constrained_project]
198200

199-
skip = [p.name for p in projects if not Path(p.path).is_absolute()]
201+
skip = [p.name for p in projects if not self.app_config.is_locally_syncable(p.name, p.path)]
200202
if skip:
201203
projects = [p for p in projects if p.name not in skip]
202-
logger.debug(f"Skipping projects without an absolute local path in watch cycle: {skip}")
204+
logger.debug(f"Skipping projects that are not locally syncable in watch cycle: {skip}")
203205

204206
return list(projects)
205207

tests/services/test_initialization.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,7 @@ async def test_initialize_file_sync_skips_project_with_non_absolute_path(
265265

266266
await initialize_file_sync(updated, quiet=True)
267267

268-
skip_logs = [m for m in infos if "without an absolute local path" in m]
268+
skip_logs = [m for m in infos if "not locally syncable" in m]
269269
assert skip_logs, "expected a skip log for the empty-path project"
270270
assert "empty-path" in skip_logs[0]
271271
assert "good" not in skip_logs[0]

tests/sync/test_watch_service_reload.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,47 @@ async def fake_write_status():
279279
assert seen_project_names == [["local-project"]]
280280

281281

282+
@pytest.mark.asyncio
283+
async def test_run_filters_orphan_db_project_absent_from_config(monkeypatch, tmp_path):
284+
"""A DB row not present in config is skipped even with an absolute path.
285+
286+
Config is the source of truth. Reconciliation normally deletes orphan rows,
287+
but if it is skipped or fails a stale row could remain; watching it would
288+
mutate a directory the user already removed from config.
289+
"""
290+
config = BasicMemoryConfig(
291+
watch_project_reload_interval=1,
292+
projects={
293+
"local-project": {"path": str(tmp_path / "local"), "mode": "local"},
294+
},
295+
)
296+
repo = _Repo(
297+
projects_return=[
298+
Project(id=1, name="local-project", path=str(tmp_path / "local"), permalink="local"),
299+
# Absolute path, but no matching entry in config -> stale/orphan row.
300+
Project(id=2, name="orphan", path=str(tmp_path / "orphan"), permalink="orphan"),
301+
]
302+
)
303+
watch_service = _watch_service(config, repo)
304+
305+
seen_project_names: list[list[str]] = []
306+
307+
async def watch_cycle_stub(projects, stop_event):
308+
seen_project_names.append([p.name for p in projects])
309+
watch_service.state.running = False
310+
stop_event.set()
311+
312+
async def fake_write_status():
313+
return None
314+
315+
monkeypatch.setattr(watch_service, "_watch_projects_cycle", watch_cycle_stub)
316+
monkeypatch.setattr(watch_service, "write_status", fake_write_status)
317+
318+
await watch_service.run()
319+
320+
assert seen_project_names == [["local-project"]]
321+
322+
282323
@pytest.mark.asyncio
283324
async def test_run_continues_after_cycle_error(monkeypatch, tmp_path):
284325
config = BasicMemoryConfig(

tests/test_config.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1245,6 +1245,30 @@ def test_set_project_mode_local_resets_to_default(self):
12451245
assert config.projects["research"].mode == ProjectMode.LOCAL
12461246
assert config.get_project_mode("research") == ProjectMode.LOCAL
12471247

1248+
def test_is_locally_syncable_true_for_config_project_with_absolute_path(self, tmp_path):
1249+
"""A project in config with an absolute path is locally syncable."""
1250+
abs_path = str(tmp_path / "research")
1251+
config = BasicMemoryConfig(projects={"research": ProjectEntry(path=abs_path)})
1252+
assert config.is_locally_syncable("research", abs_path) is True
1253+
1254+
def test_is_locally_syncable_false_for_empty_path(self):
1255+
"""An empty path resolves to cwd, so it is never locally syncable (#949)."""
1256+
config = BasicMemoryConfig(projects={"empty": ProjectEntry(path="")})
1257+
assert config.is_locally_syncable("empty", "") is False
1258+
1259+
def test_is_locally_syncable_false_for_relative_path(self):
1260+
"""A relative (slug) path, as used by cloud-only projects, is not syncable."""
1261+
config = BasicMemoryConfig(projects={"cloud": ProjectEntry(path="cloud-slug")})
1262+
assert config.is_locally_syncable("cloud", "cloud-slug") is False
1263+
1264+
def test_is_locally_syncable_false_for_orphan_not_in_config(self, tmp_path):
1265+
"""A DB row absent from config is not syncable even with an absolute path.
1266+
1267+
Config is the source of truth; stale rows must not be synced (#949).
1268+
"""
1269+
config = BasicMemoryConfig(projects={})
1270+
assert config.is_locally_syncable("orphan", str(tmp_path / "orphan")) is False
1271+
12481272
def test_cloud_api_key_defaults_to_none(self):
12491273
"""Test that cloud_api_key defaults to None."""
12501274
config = BasicMemoryConfig()

0 commit comments

Comments
 (0)