Repository navigation
refactor(mcp): remove read_note, edit_note, workspaces and chatgpt tool modules - #1964
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…ol modules Basic Memory 1.0 publishes only the bm_* tools, and Cloud no longer imports these modules (basic-memory-cloud#2857). Delete them with the code only they reached: the link_resolver workspace-prefix detection, routes_to_default_project and add_project_metadata, KnowledgeClient.place_note, and the strict-routing options of resolve_project_and_path with UnresolvedProjectRouteError. The read_note title-lookup paging loop goes with read_note. Port their tests to bm_read, bm_edit, bm_search and bm_context, and drop the ones that covered legacy-only behavior (title-search fallback, auto-create, disk recovery, legacy output shapes). Telemetry tests now use logfire's capfire instead of patching logfire.span. Closes #1958 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMYVL1x91B8kJxy7nn2GU8 Signed-off-by: phernandez <paul@basicmachines.co>
The probe ran the deleted ChatGPT search pagination test. test_search_pagination does the same through the production MCP server: 15 bm_write calls, then paged bm_search on SQLite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMYVL1x91B8kJxy7nn2GU8 Signed-off-by: phernandez <paul@basicmachines.co>
Member
Author
|
@codex review |
phernandez
force-pushed
the
refactor/remove-cloud-only-legacy-tools
branch
from
October 29, 2026 19:12
ca39b0b to
9fc53d6
Compare
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
phernandez
added a commit
that referenced
this pull request
Oct 29, 2026
…#1963) ## Summary **Stacked on #1964** (`refactor/remove-cloud-only-legacy-tools`), which removes `read_note`, the last core caller of `search_notes`. Merge #1964 first. Two commits: 1. **test(mcp): port search tests to bm_search.** Ports the search tests from the retired `search_notes` function to the published `bm_search` tool. 2. **refactor(mcp): remove search_notes.** `bm_search` is now the only caller, so `search.py` keeps only the shared query pipeline (about 1,100 src lines deleted). - Deleted `search_notes`, `_format_search_error_response`, `_format_service_unavailable_response`, `_format_search_markdown`, `_NO_SEARCH_CRITERIA_MESSAGE`, `METADATA_FILTERS_DESCRIPTION`, and the `tools/__init__` re-export. - `_search_all_projects` no longer takes `output_format`. It always returns the merged rows. `MultiProjectRan` carries `payload`, and `MultiProjectFailed` carries only `reason`. This removes `bm_search`'s `# pragma: no cover` text branch. - Dropped the `status` parameter of `_build_search_query` and the `tags` parameter of `split_tag_shorthand`; only `search_notes` passed them. `split_tag_shorthand` now returns tags in query order instead of set order. - Deleted `utils.strict_search_tags` (the tags coercer for `search_notes`) and its tests in `tests/test_coerce.py`. - Ported the remaining callers: `test_index_readiness.py`, which #1962 also ports; the parity test in `test_tool_posix.py`, which #1960 deletes; and the `search_notes` entries in `test_tool_contracts.py` and `implementation_mcp.py`. - Added tests for `bm_search` paths that were covered only incidentally before: - the explicit-scope refusal; - the `(more matches from line N)` hint; - the semantic excerpt in text output; - non-Markdown hits dropped by `lines`; - invalid and duplicate project refs; - workspace permalink qualification. ## Files | Before | After | |---|---| | `tests/mcp/test_tool_search.py` | `tests/mcp/test_tool_bm_search_query.py` (new) | | `tests/mcp/test_mcp_note_type_normalization.py` | folded into `test_tool_bm_search_query.py` | | `tests/mcp/test_search_total_exactness.py` | folded into `test_tool_bm_search_query.py` | | `tests/mcp/test_tool_search_temporal.py` | `tests/mcp/test_tool_bm_search_temporal.py` | | `tests/mcp/tools/test_search_notes_multi_project.py` | `tests/mcp/tools/test_bm_search_multi_project.py` | | `tests/mcp/tools/test_search_notes_multi_project_temporal.py` | folded into `test_bm_search_multi_project.py` | | `tests/mcp/tools/test_cjk_search_guidance.py` | folded into `test_bm_search_multi_project.py` | | `tests/api/v2/test_cjk_search_guidance.py` | its MCP assertions now call `bm_search` | | `tests/mcp/test_tool_bm_search.py` | two tests that replaced `_search_all_projects` now run the real path or moved to the multi-project file | ## Ported to real code paths These tests used to stub `get_project_client`, `resolve_project_and_path` and `SearchClient` and inspect the captured payload. They now do one of two things: - Run `bm_search` end to end over notes written with `bm_write`: title and permalink modes, note types including snake_case canonicalization, items, categories, `since`, the `note_type` filter alias, tag shorthand, total exactness per mode, default hybrid mode, Boolean queries defaulting to text, configured `default_search_type`, never-indexed versus indexed misses, and comma or JSON list strings through the MCP layer. - Call the shared pure helpers that `bm_search` uses on every request: `_build_search_query`, `split_tag_shorthand` and `_search_type_for`. The valid-time doc tests now check `bm-search(3)` instead of the `search_notes` docstring. ## Remaining monkeypatches, and why - **Multi-project tests:** several databases only exist in Cloud. These tests stub the per-database boundary: project discovery, the per-workspace `get_client`, `ScopedSearchClient` and the readiness probe. The merge, paging, attribution and failure handling run for real. A new test, `test_hits_from_two_local_projects_name_their_project`, runs the whole path with no stubs across two real local projects. - **"Filter ran nowhere" temporal test:** stubs only `ScopedSearchClient`, to stand in for a server older than SPEC-82. - **Keyword-only host:** two tests patch `semantic_runtime_available`, because sqlite-vec loads in the test environment. - **Default-mode tests:** these install the MCP container the way the server lifespan does. An earlier in-process MCP session leaves its container behind with a stale config, so each test sets its own. ## Dropped, with reasons - **Error formatting** (`TestSearchErrorFormatting`, `TestSearchToolErrorHandling`, invalid `search_type` returned as a string): `bm_search` raises tool errors instead of returning guidance markdown. Its refusals are covered in `test_tool_bm_search.py`. - **`_format_search_markdown` tests, and the valid-time label in markdown output:** this is the legacy text format. `bm_search` renders with `_format_results`. - **Workspace memory:// routing through `search_notes`:** `bm_search` routes memory:// URLs through `open_project_target`. That path is covered by `test_note_targets.py` and the project-qualified memory URL test. - **`search_notes` parameter-annotation tests and `tags=` tests** (comma-split, invalid types, direct-call CLI regression): `bm_search` has no `tags` or `status` parameters, and no CLI calls `search_notes` any more. The equivalent `bm_search` list parameters are tested through MCP. - **Multi-project opt-in:** `bm_search` requires an explicit scope, and that refusal is already tested. - **Redundant cases:** plain text search, the project-prefixed memory URL and `page_size=1` pagination are already covered in `test_tool_bm_search.py`. ## Coverage `search.py` and `ranked_search.py` are both at 100%, measured over the touched test files. ## Verification - `just fast-check` - `just test-unit-sqlite -k "search or test_index_readiness or test_tool_contracts or test_coerce or ..."`: 1146 passed. One failure, `test_tokenizing_scales_linearly_with_query_length`, is a timing test; it passes when rerun on its own. - `just test-unit-postgres -k "bm_search or test_cjk_search_guidance or test_index_readiness or test_tool_contracts or test_coerce or bm_search_parity"`: all pass, after making one new test backend-neutral. - `just test-int-sqlite -k search`: 64 passed 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01CMYVL1x91B8kJxy7nn2GU8 --------- Signed-off-by: phernandez <paul@basicmachines.co> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
phernandez
added a commit
that referenced
this pull request
Oct 29, 2026
## Why `just release v1.0.0` pre-flight-checks for a `## v1.0.0` heading on `main`, so the changelog has to land first. ## What changed `CHANGELOG.md`: the Unreleased section becomes `## v1.0.0 (2026-10-29)`. Its existing items are kept, and the 60 or so commits since #1871 are added: - **Breaking:** - the MCP surface is only `bm_*`, with the old tools deleted (#1944, #1947, #1959, #1962–#1964) - `project` is required (#1934) - the ChatGPT adapters moved to Cloud - `bm tool` mirrors the tools (#1943) - the POSIX verbs are rebuilt and `bm tail` is removed (#1960) - cloud sync/bisync are removed (#1953) - `bm schema` project resolution (#1959) - LiteLLM is an optional extra - **Features:** text-file line ranges in `bm_read`; chat imports return their file paths. - **Bug fixes:** including the three found in the 1.0 live test (#1971, #1972, #1974), with contributor credits for @Flynn-Kawolsky (#1957), @SWSAmor (#1951) and @MbappeWU (#1948). - **Security:** the CodeRabbit deep-scan hardening batch. I checked the entries against PR bodies rather than titles, and corrected #1954, #1948 and #1967 to describe what they actually fixed. The date is today; change it if the tag lands tomorrow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01CMYVL1x91B8kJxy7nn2GU8 Signed-off-by: phernandez <paul@basicmachines.co> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Deletes the legacy MCP tool modules that Basic Memory 1.0 no longer publishes and that Cloud no longer imports:
mcp/tools/read_note.py(read_note), replaced bybm_readmcp/tools/edit_note.py(edit_noteand its helpers), replaced bybm_editmcp/tools/workspaces.py(list_workspaces), replaced bybm_project(command="list")mcp/tools/chatgpt_tools.py(search,fetch), which moved into Cloudmcp/tools/__init__.pydrops their re-exports. The read_note title-lookup paging loop goes withread_note.Closes #1958
Merge order: this must merge after basicmachines-co/basic-memory-cloud#2857 and a Cloud re-pin of core. Until then Cloud still imports these modules.
Dead code removed with them
Nothing else in core imported the four modules. Once they were gone, these had no callers in core or in Cloud (checked against the #2857 branch):
services/link_resolver.py:is_workspace_qualified_plain_identifier,detect_project_from_workspace_identifier_prefix(only edit_note used them), plus their 6 unit testsmcp/project_context_identifiers.py:routes_to_default_project,add_project_metadata, and their re-exports inproject_context.pymcp/clients/knowledge.py:KnowledgeClient.place_note(only edit_note's auto-create used it)mcp/project_context.py: thestrict_project_routing,allow_missing_project_fallbackandcache_resolved_projectoptions ofresolve_project_and_path, andUnresolvedProjectRouteError. Only edit_note passed them; Cloud's review tools callresolve_project_and_pathwithout them. The threetest_project_context_scoped_key.pytests for those options go too.Kept on purpose:
KnowledgeClient.index_file(document ingestion uses it),mcp/client_info.is_openai_mcp_client(Cloud's ChatGPT tools import it), the man pageschatgpt-search(3)/chatgpt-fetch(3)and the hard-coded Cloud tool list in the section-3 man test (Cloud still registers those tools), and the_select_project,_selection_guide,_project_route_argumenthelpers inprompts/getting_started.py.The API's
/knowledge/placementand/knowledge/index-fileendpoints are unchanged.Tests ported to the published surface
test_tool_read_note.pytest_tool_bm_read.py: unicode round trip, memory URL with project prefix (by name and by external id), team/personal workspace permalinks (complete permalink, no project-prefix config, short legacy permalink plain and workspace-qualified), traversal/absolute/home/memory-URL identifiers read nothing, missing external-id line read is NOT_FOUND, line read past the end reports the length (path and external id, text and JSON), read checksum guards a bm_write overwritetest_read_note_request_counts.pytest_note_reads.py; keeps the threeread_note_json_by_external_idhelper tests, plus the CRLF frontmatter andformat_note_metadatatests moved fromtest_tool_read_note.py. The bm_read request counts (one resolve, one entity read, no resource read) are intest_tool_telemetry.pytest_line_scanning.pyread_notecalls nowbm_read; invalid bounds go through MCP schema validationtest_unknown_project_routing.pybm_read+bm_searchtest_workspace_permalink_resolution.pybm_edit+bm_readtest_tool_edit_note.pytest_tool_bm_edit.py: replace_section with and without subsections (#1012), insert before/after a section, missing section changes nothing (all three section commands), expected_replacements count, whitespace find_text, identifier spellings (permalink, title, folder/title, path, memory URL), memory URL with project prefix, near-miss title edits nothing, prefixed traversal identifiers write nothing, team workspace memory URL keeps the complete permalink, repeated edits keep the permalink (#170), empty append, explicit nulls via MCP, appended observations/relations are indexedtest_tool_guidance.pybm_editrefusal points tobm_readtest_wiki_after_api_writes.pybm_edittest_tool_workspace_management.pytest_workspace_provider.py; keeps theget_available_workspacesprovider teststest_tool_telemetry.pycapfire(no monkeypatch) forbm_read,bm_edit,bm_search,bm_context: exact API request counts and the routed projecttest_required_project.pytest_tool_json_output_modes.pytest_tool_build_context.py,test_remote_path_validation.pyread_notecall is nowbm_readtest_refusals.py_revision_conflict_detail)test_project_context.pyresolve_project_and_pathkeeps amemory://<workspace>/<project>/...route inside that workspace. read_note's workspace tests were the only coverage of that branch, which search, build_context and Cloud's review tools still reachTests dropped (legacy-only behavior)
test_read_note_title_*,test_read_note_returns_related_results_*,test_read_note_forwards_pagination_*,test_read_note_rejects_non_positive_pagination,test_exact_title_fallback_keeps_line_scan,test_read_note_memory_url_fallback_uses_search_tool_normalization, the search-fallback request-count tests).bm_readresolves exactly and refuses a miss with NOT_FOUND pointing tobm_search; there is no fallback.bm_readraises NOT_FOUND._compose_workspace_project_route,detect_project_from_*call-skipping tests, explicitworkspace=argument, stale active-project cache, plain workspace-qualified write guidance).bm_*routing goes throughnote_targets.open_project_target, covered bytest_note_targets.py,test_project_path_routing.pyandtest_project_context.py(incl.test_get_project_client_clears_stale_cached_project_for_workspace_route).bm_editnever creates a note (test_missing_note_is_not_created)._resolve_after_disk_recovery.bm_editdoes not index on a miss.# Edit Failedheadings,fileCreated,[Project: ...]footer,EDIT_OUTCOME_UNKNOWN,SECURITY_VALIDATION_ERROR).bm_edituses the shared refusal codes, covered intest_tool_bm_edit.pyandtest_refusals.py.checksum: unknownalthough the checksum is persisted #1586) via a patched PATCH response.NoteChangereportsdb_checksum; covered intest_note_writes.py.bm_project(command="list")groups projects by workspace throughlist_memory_projects, covered intest_tool_project_management.py.search/fetchunit tests, integration tests and the connector fetch routing test intest-int/mcp/test_adapter_project_routing.py. Cloud owns these tools now.Verification
just fast-checkjust fast-test(testmon-impacted tests)just fast-test test-int -m "not semantic and not benchmark"andjust test-int-sqlite -k "adapter_project_routing or edit_note_integration or read_note_integration or workspace"note_reads,note_targets,project_context,project_context_identifiers,link_resolver,clients/knowledge,tools/read,tools/edit) compared againstmainover the same test selection: no line that was covered before is uncovered now🤖 Generated with Claude Code
https://claude.ai/code/session_01CMYVL1x91B8kJxy7nn2GU8