Skip to content

feat: Fix bm CLI runtime defects and audit regressions#596

Merged
phernandez merged 1 commit intomainfrom
codex/cli-audit-20260220-184102
Feb 21, 2026
Merged

feat: Fix bm CLI runtime defects and audit regressions#596
phernandez merged 1 commit intomainfrom
codex/cli-audit-20260220-184102

Conversation

@phernandez
Copy link
Member

@phernandez phernandez commented Feb 21, 2026

Summary

  • Fixes bm reset shutdown hang in non-interactive flows
  • Fixes SQLite search-notes query + metadata filter MATCH context error
  • Fixes bm cloud upload --no-sync WebDAV routing path
  • Improves cloud API-key auth failure remediation messaging
  • Adds focused regressions across CLI/repository/service/tool layers (including Postgres coverage for search filters)

Validation

  • Targeted regression suite passes
  • Postgres search regression slice passes
  • Manual runtime verification completed for reset and upload paths

Runtime notes (hang fixes)

  • bm reset and bm reset --reindex previously hung after printing completion when run non-interactively (for example with piped y).
  • Root cause was migration-time temporary engine/thread lifecycle not being disposed cleanly.
  • Fixed in this PR and manually re-verified with:
    • printf 'y\n' | uv run bm reset
    • printf 'y\n' | uv run bm reset --reindex
  • Both now exit cleanly with exit code 0.

Follow-up after cloud deploy

  • bm project info --cloud must be re-verified after deploying corresponding cloud backend changes (proxy/API alignment).
  • Current hosted cloud still returns: Internal proxy error: Project '<name>' not found in configuration for /proxy/v2/projects/{id}/info.

Fixes reset shutdown hang, sqlite search-notes metadata MATCH context, cloud upload --no-sync routing, and cloud API-key error remediation; adds focused regression tests (including Postgres coverage for search filters).

Note: cloud project-info behavior (bm project info --cloud) still depends on cloud deployment state and must be re-verified after deploying the corresponding backend changes.
Signed-off-by: phernandez <paul@basicmachines.co>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 21, 2026

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


PR Review: Fix bm CLI runtime defects and audit regressions

Tasks:

  • Gather PR context and understand changes
  • Review changed source files
  • Review new test files
  • Check against team checklist
  • Post final review

Review Summary

This PR delivers five targeted bug fixes with focused regression tests for each. The changes are well-scoped and follow the project's "minimize diffs" principle. Overall quality is high — a few observations below.


Code Quality & Standards ✅

  • Follows Basic Memory's coding conventions in CLAUDE.md
  • Python 3.12+ type annotations and async patterns
  • SQLAlchemy 2.0 best practices
  • FastAPI and Typer conventions followed
  • 100-character line length maintained
  • No commented-out code blocks

Testing & Documentation ✅

  • Unit tests for new functions/methods (7 new test files)
  • Integration test added for metadata+text combined search
  • Edge cases covered
  • 100% coverage maintained with appropriate # pragma: no cover usage

Basic Memory Architecture ✅

  • Local-first architecture preserved
  • MCP tools remain atomic/composable
  • No database schema changes (no migration needed)

Security & Performance ✅

  • No hardcoded secrets
  • Input validation maintained
  • No sensitive data in logs

Observations

1. BM25 Relevance Scoring May Degrade with Combined Filters

File: src/basic_memory/repository/sqlite_search_repository.py:760-768

The fix correctly solves the "unable to use function MATCH in the requested context" error by moving MATCH clauses into a subquery. However, bm25(search_index) in the outer query requires an active FTS5 MATCH context on search_index to return meaningful scores:

-- Outer query has no MATCH on search_index — bm25() will return NULL or 0
SELECT ..., bm25(search_index) as score
FROM search_index JOIN entity ...
WHERE ... AND search_index.rowid IN (SELECT rowid FROM search_index WHERE title MATCH :text)
ORDER BY score ASC

When metadata_filters are present, all results in the combined query will get score = NULL, losing relevance ordering. This is a correctness trade-off — the fix prevents crashes, but ranking is effectively disabled for this query path. Worth noting in a follow-up issue or comment if this matters for UX.

Fix this →

2. Debug Log May Fail on Non-JSON Success Responses

File: src/basic_memory/mcp/tools/utils.py:444

logger.debug(f"response: {response.json()}")

This line only runs on successful responses, but response.json() will raise a JSONDecodeError if the success response body is not JSON (e.g., binary content or plain text). This is a pre-existing line but worth guarding:

logger.debug(f"response: {response.text[:200]}")  # safer

3. Minor: Double .lower() Call in Error Check

File: src/basic_memory/cli/commands/command_utils.py:101-103

if "internal proxy error" in error_text.lower() and "not found in configuration" in (
    error_text.lower()
):

error_text.lower() is computed twice. Minor style nit — could assign to a local variable. Functionally correct.

4. project.py Fix: except typer.Exit: raise

File: src/basic_memory/cli/commands/project.py

The added except typer.Exit: raise correctly prevents typer.Exit(1) from being caught and re-raised as Error getting project info: 1. This is the right fix for the double-error-message bug.

5. test_tool_utils_cloud_auth.py - HTTPStatusError(request=None)

File: tests/mcp/test_tool_utils_cloud_auth.py:21-25

The mock raises HTTPStatusError(message=..., request=None, response=self). Passing request=None is technically outside the httpx contract and may behave differently across httpx versions. If this test becomes flaky, consider using httpx.Request("POST", "http://test/") as a real request object. Functionally fine for now.

6. run_with_cleanup + shutdown_db Missing Test Coverage

File: src/basic_memory/cli/commands/command_utils.py:23-42 and src/basic_memory/db.py:411-418

Both run_with_cleanup and shutdown_db carry # pragma: no cover markers (shutdown_db on db.py:411). The subprocess-based test_db_reset_exit.py tests exercise run_with_cleanup indirectly — this is acceptable per CLAUDE.md given the runtime-mode-dependent nature of shutdown paths.


Overall Assessment

The fixes are correct and the test coverage is solid. The main concern worth tracking is #1 (BM25 scoring with metadata filters) — it's a functional correctness issue though not a crash. All other observations are minor. Recommend approving pending acknowledgement of the BM25 scoring trade-off.

@phernandez phernandez changed the title Fix bm CLI runtime defects and audit regressions feat: Fix bm CLI runtime defects and audit regressions Feb 21, 2026
@phernandez phernandez merged commit b86dd6f into main Feb 21, 2026
42 of 46 checks passed
@phernandez phernandez deleted the codex/cli-audit-20260220-184102 branch February 21, 2026 04:59
@phernandez phernandez added this to the v0.19.0 milestone Feb 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant