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>
The absolute-path sync guard from the previous commit now checks every
project's path, not just cloud-mode ones. Three existing watch-selection
tests hardcoded POSIX paths like "/tmp/alpha", which are absolute on
Linux/macOS but not on Windows (no drive letter), so the guard filtered
them out and the tests failed on windows-latest.
Build the project paths from the tmp_path fixture so they are absolute on
every platform. Production paths are unaffected: real local projects are
always resolved to OS-absolute paths at creation.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
A project entry in config.json with an empty path (e.g. `{"path": ""}`)
caused background sync and the watch service to adopt the process cwd as
the project root, injecting Basic Memory frontmatter into unrelated
markdown files.
The existing guards only skipped a project when get_project_mode()
returned CLOUD. But ProjectEntry.mode defaults to LOCAL, so an empty- or
relative-path entry without an explicit mode slipped through, and
Path("") resolves against the current working directory.
Gate local sync and watching on the path itself: any project whose path
is not absolute is excluded, regardless of mode. Legitimate local
projects are always resolved to absolute paths at creation, and cloud
projects with a real local bisync copy keep their absolute path and are
still synced/watched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
The signature contract test pins exact tool parameters; update it for
the new workspace parameter.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
write_note now accepts workspace= alongside project= and project_id=,
matching the parameter surface and docstring guidance already present in
edit_note. The _compose_workspace_project_route helper is added locally
(mirroring the edit_note pattern) so agents can route writes to same-named
projects across workspaces using workspace/project qualified syntax.
Closes#882
Other write-path tools that share the same gap (move_note, delete_note)
are noted here but left for a separate PR to keep this change focused.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
On a brand-new config dir, model_post_init bootstraps a 'main' default in
config.json but the one-shot CLI path never runs the server-lifespan
reconciliation that would create its database row, so the first read fails
with a bare "Project not found: 'main'" — which reads as a broken install
rather than a missing first-run step. When resolution misses and the
projects table is empty, the 404 now names the setup command.
Follow-up to #974/#985/#987 (which repair the default during project add
but cannot help when the first action is a read). Refs #974.
Signed-off-by: phernandez <paul@basicmachines.co>
Follow-up to #985 (the #974 fix by @rudi193-cmd), addressing the codex P2
raised on #987: when config's default_project has no database row but the
database still holds a valid default of its own, promoting the just-added
project would silently steal that default. The repair now repoints config
at a surviving database default — matching synchronize_projects, which
treats the database default as authoritative — and promotes the added
project only when no usable database default exists (no default at all,
or one unknown to config, which set_default_project rejects and
reconciliation deletes).
Also removes an unused ProjectConfig import from the #985 regression test
(strict ruff failure; CI's --fix lint masked it).
Refs #974
Signed-off-by: phernandez <paul@basicmachines.co>
When add_project promotes a new project because the configured default is missing from the database, return the persisted default flag instead of echoing the request flag.
Signed-off-by: rudi193-cmd <rudi193@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Fresh CLI installs auto-create main in config but never sync it to the
projects table; the first project add now becomes default when the
configured default has no DB row. Fixes#974.
Signed-off-by: rudi193-cmd <rudi193@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Every basic-memory CLI invocation paid roughly 2 seconds of module-import
cost before any work started, which blew the Claude Code plugin's
SessionStart hook budget on cold machines (#886). The cost came from
module-level imports that pulled the entire server stack into CLI startup:
- mcp/async_client.py imported FastAPI at module level, so every consumer
of get_client() loaded FastAPI even for cloud-routed or help-only paths.
- mcp/clients/*.py imported call_* helpers from basic_memory.mcp.tools.utils,
which executes the whole tools package __init__ — every MCP tool module
plus fastmcp and the mcp SDK.
- mcp/project_context.py imported fastmcp.Context and ToolError eagerly.
- CLI command modules (tool, ci, schema) imported MCP tool functions at
module level; db and the import_* commands pulled SQLAlchemy/Alembic and
the markdown/file-service stack; status/doctor/orphans/command_utils
imported ToolError (the mcp SDK) and basic_memory.db.
- schemas/base.py imported dateparser (~0.13s) for one helper function.
The fix only defers imports to the point of use (no behavior changes):
FastAPI now loads inside _resolve_local_asgi_database alongside the
existing lazy api.app import, so it is only paid when a request actually
routes through the in-process ASGI transport; the typed clients import
call_* per method; project_context uses PEP 563 annotations with Context
under TYPE_CHECKING; the CLI command modules import their heavy
dependencies inside the command bodies. Tests that patched the old
module-level aliases now patch the source modules instead.
Measured on a warm cache (python -X importtime / wall time):
- import basic_memory.cli.main: 1.92s -> 0.45s
- bm --help: 2.40s -> 0.52s
- bm tool search-notes --help: 2.40s -> 0.86s
A regression test asserts that importing the CLI entry module with full
command registration leaves fastapi, sqlalchemy, alembic, fastmcp, mcp,
basic_memory.api.app, basic_memory.db, basic_memory.markdown,
basic_memory.mcp.tools, and basic_memory.services out of sys.modules.
Fixes#886
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
Root cause: entity, observation, and relation rows in search_index carry
ids from independent auto-increment sequences, so rows of different types
routinely share the same numeric id (guaranteed in young databases).
_search_vector_only parsed each vector hit's chunk_key (e.g. 'entity:4:0')
but discarded the type, and _fetch_search_index_rows_by_ids keyed its
result dict by bare row.id with no type discrimination. Whichever row the
database returned last clobbered the other in the dict; the clobbered hit
then hydrated against the wrong row or found None and was silently
dropped from results. The FTS-filter branch already guarded this with
(id, type) tuples, but the primary vector lookup path and the hybrid
fusion maps missed the same treatment.
Fix: introduce a SearchIndexKey = tuple[str, int] alias and key every map
in the vector/hybrid retrieval path by (type, id) — the similarity and
chunk maps in _search_vector_only, the _fetch_search_index_rows_by_ids
result, the FTS-filter allowed keys, and the rows/fts/vec/fused score
maps in _search_hybrid. The SQL stays unchanged; bare ids are deduped
before the IN query and rows are discriminated by row.type when building
dict keys.
Tests: end-to-end SQLite regression test indexes an entity row and a
relation row sharing id 7, syncs vectors for both, and asserts vector
search returns both rows (and that the entity survives a
search_item_types filter); a hybrid fusion unit test asserts an entity
and relation sharing id 1 stay distinct with single-source scores. Both
fail without the fix. Existing mocked vector tests updated for tuple keys.
Fixes#982
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
Root cause of the test_mcp_sse_forces_local KeyError('FORCE_LOCAL') flake
(#940, Python 3.14 leg): the stdio variants of the mcp routing tests invoke
`bm mcp`, which starts a real background auto-update daemon thread before
mcp_server.run (the tests only mock run). That thread hits PyPI and then
rewrites config.json (auto_update_last_checked_at) with an in-place
Path.write_text, which truncates the file before writing. If that write lands
while a later test's CLI invocation is reading config.json (the app callback's
CliContainer.create()), load_config() sees empty/partial JSON and raises
SystemExit. CliRunner.invoke swallows it, the mocked mcp_server.run never
executes, and the test dies with KeyError on env_at_run['FORCE_LOCAL'] —
exactly the observed CI failure shape. Nothing is 3.14-specific; that leg
only shifted the timing.
The same torn write is user-visible in production: the MCP stdio server
re-reads config.json on mtime change and load_config() exits the process on
invalid JSON if it races a CLI save.
Fix: save_basic_memory_config writes a per-process/per-thread sibling temp
file and publishes it with os.replace, so readers always observe either the
old or the new complete document. The regression test injects an interrupted
write and asserts the published config stays untouched; it fails against the
old in-place write.
Test hardening: the stdio routing tests stub run_auto_update so no PyPI call
or config write leaks across tests, and all four transport tests now assert
result.exit_code == 0 so a future pre-run failure surfaces its real error
instead of a KeyError.
Refs #940
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
Root cause of the test_sync_entity_circular_relations CI failure (#940,
len(entity_b.outgoing_relations) == 0): the in-memory SQLite URL
(sqlite+aiosqlite://) falls back to SQLAlchemy's StaticPool, which hands the
same DBAPI connection to every concurrently checked-out session. Concurrent
asyncio tasks therefore share one SQLite transaction scope. A rollback issued
through one session — scoped_session's exception handler, or the pool's
reset-on-return at connection checkin (~40 real ROLLBACKs per sync, measured)
— also rolls back any other task's executed-but-uncommitted statements.
During batch indexing, per-file tasks run concurrently (asyncio.gather bounded
by index_entity_max_concurrent). If a sibling session's checkin ROLLBACK lands
between one task's relation INSERT and its COMMIT, the relation row is silently
erased: no error is raised, the sync reports success. The final sync-level
resolve_relations() pass cannot heal this because it only re-queries rows with
to_id IS NULL — the destroyed INSERT leaves no row at all.
Fix: give MEMORY-type engines a single-connection AsyncAdaptedQueuePool
(pool_size=1, max_overflow=0). The lone connection keeps the in-memory
database alive for the engine's lifetime (the reason StaticPool was used),
while the blocking checkout serializes sessions at transaction granularity,
restoring the isolation the repositories assume. File-based SQLite and
Postgres engines are unchanged; production never uses MEMORY engines.
The regression test pins the invariant directly: a session that rolls back in
one task must never destroy another task's uncommitted writes. It fails
deterministically against StaticPool and passes with the serialized pool.
tests/repository/test_entity_repository.py held a scoped session open while
calling a repository method that opens its own session — tolerated on a shared
connection, a deadlock under a serialized pool — so the nested call moved out
of the session block.
Refs #940
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
The edit_note docstring (and write_note in PR #964) advertise that the
workspace segment of a "workspace/project" route may be a slug, name, or
tenant_id, but resolve_workspace_project_identifier() only matched against
WorkspaceInfo.slug. Cloud routes using a display name or tenant UUID failed
with "Workspace ... was not found" despite the documented contract.
Extend the first-segment matching (only the matching logic; the overall
resolution flow is unchanged) to honor, in priority order:
1. slug (casefold) — unchanged, checked first so working routes keep meaning
2. tenant_id — exact match on the opaque id
3. display name (casefold) — fails fast on collisions, listing candidate slugs
A name that collides with another workspace's slug resolves to the slug owner.
Unknown identifiers raise a not-found error naming the forms that were tried.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
Adds human-readable `title` and categorization `tags` to all ~24 @mcp.tool
decorators across the MCP tools package. FastMCP 3.3.1 (pinned in pyproject.toml)
supports both fields natively. Tags used: notes, search, projects, cloud, schema,
navigation, canvas, ui.
Extends test_tool_contracts.py with an async test that asserts every registered
tool has a non-empty title and at least one tag to prevent future regressions.
output_schema is explicitly deferred as a follow-up (phase 2): it requires
per-tool design decisions about which tools reliably return structured JSON and
how to handle tools that return str|dict depending on output_format.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
Codex review of PR #962 identified two real issues:
1. CLI bypass: the BeforeValidator(parse_str_list) on note_types, entity_types, and
categories only fires through MCP/Pydantic validation. The CLI path in
cli/commands/tool.py calls search_notes() directly, so `bm tool search-notes
--type note,task` arrived as note_types=["note,task"] and matched nothing.
Fix: add in-body parse_str_list() normalization for all three params (mirroring
the existing parse_tags() call for tags on the same code path).
2. Silent stringify: parse_str_list used str(raw) in the list branch, so [42] became
["42"] before Pydantic saw it, accepting invalid input as a no-result search
instead of rejecting it. Fix: guard against non-string list elements and return
the original value unchanged so Pydantic rejects it with a clear error.
Tests added: annotation-level split tests for note_types/entity_types/categories,
non-string-element rejection tests, async direct-call regression for note_types,
and unit-level parse_str_list non-string list tests in test_coerce.py.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
Add `parse_str_list` to utils.py — like `parse_tags` but without stripping
'#' — and wire it as the BeforeValidator for note_types, entity_types, and
categories in search_notes. This makes passing "note,task" or
'["note","task"]' work correctly instead of being wrapped as a single literal
value by coerce_list.
coerce_list is left unchanged; canvas and other callers that depend on its
wrap-single-string behaviour are unaffected.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
The v2 set_default_project_by_id handler fetched the current default
solely to echo it as old_project in the response, and raised 404
"No default project is currently set" when none existed. That guard
(marked # pragma: no cover, never tested) made the bootstrap/recovery
case impossible: with no default row in the DB, `bm project default
<name>` always failed -- yet that is exactly the command you reach for
when no default is set.
ProjectStatusResponse.old_project is already Optional, so the guard
served no schema requirement. Remove it and build old_project only
when a previous default exists, else pass None.
Add a regression test that clears is_default, sets a default via the
endpoint, and asserts 200, old_project is None, the new project is
default, and a follow-up read-back returns it.
Closes#975
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
has_frontmatter(), parse_frontmatter(), and remove_frontmatter() detected
frontmatter by substring/split (`content.startswith("---")` plus
`content.split("---", 2)`) rather than by line-anchored fences. A single-line
string that merely starts with `---` — e.g. `---\nstatus: active\n---\nBody`
where `\n` are literal backslash-n characters, a common CLI/agent input shape —
was misread as frontmatter: yaml parsed `\nstatus` as a key that got merged into
the note's YAML on disk, and the body was silently transformed.
Introduce a shared `_split_frontmatter()` helper that anchors both fences to
their own lines (`^---[ \t]*$`), tolerating leading blank lines so dedented
heredoc-style content still parses. All three public helpers now delegate to it,
preserving existing behavior for valid frontmatter (BOM stripping, empty -> {},
non-dict -> ParseError, and the existing ParseError messages callers assert on).
Closes#972
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
brew upgrade / uv tool upgrade delete the running install's files while the old process is still alive. rich defers importing rich._emoji_codes until print time and typer defers typer.rich_utils until excepthook time, so printing the post-upgrade status message crashed the exiting process with ModuleNotFoundError (and the traceback renderer crashed too). Import both before launching the upgrade subprocess, while the files still exist.
Signed-off-by: Drew Cain <groksrc@gmail.com>
Disable the silently-on semantic embedding stack in default test fixtures and deselect on-demand benchmarks from CI int jobs. int SQLite 337s -> 110s, Postgres unit ~25min -> ~13min.
Signed-off-by: phernandez <paul@basicmemory.com>
Adds additive, git-style `bm cloud push`/`pull` that are safe on shared Team workspaces (never delete on the destination; conflicts abort by default with `--on-conflict {fail|keep-local|keep-cloud|keep-both}`), and gates the destructive `bm cloud sync`/`bisync` mirrors to Personal workspaces. Closes#858. Longer-term Team-safe reconciler tracked in #862; workspace-scoped mount info (Codex P1) tracked as a follow-up.
Drew Cain