Repository navigation
refactor(cli)!: rebuild cat/grep/ls/find on bm_* tools and remove tail - #1960
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. |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4625f0f2dd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a12b1eb761
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d753204eb9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a557fbddec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
a557fbd to
80b0e41
Compare
|
@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". |
…#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>
tests/mcp/test_tool_posix.py called cat/grep/ls/find/tail as MCP tool functions, but they are no longer MCP tools; the bm shell verbs are their only callers. Port the suite to the CLI and drop the MCP-only plumbing. - test-int/cli/test_posix_commands_integration.py: real-stack bm cat/grep/ls/find/tail runs, seeded with bm tool write. - tests/cli/test_cli_posix_argument_errors.py: refusals that land before any I/O, through the CLI. - tests/mcp/test_find_meta_predicates.py: the pure predicate grammar and field projection helpers. - tests/mcp/test_tool_posix_man.py: posix_tools.man, which no CLI command reaches but tools/__init__ still re-exports. posix_tools: remove the FastMCP context parameter, the coerce_list validators on find's meta/fields, the empty-meta refusal, and the depth and tail-lines bounds that Typer already enforces. 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 shell read verbs ran on posix_tools.py, the pre-1.0 MCP tool code. They now run the published tools the way `bm tool` does (call_tool / run_tool with validate_call): cat and head call bm_read, grep calls bm_search in text mode, ls calls bm_list, find and tree call bm_find. The --meta predicate grammar moves to cli/commands/find_predicates.py and feeds bm_find's filters. - Delete posix_tools.py (cat, grep, ls, find, tail, man) and its re-exports, and the Rich/plain renderers only those verbs used. - Remove bm tail, its man1 page and references; bm tool project recent replaces it (the benchmark seed guard now uses it). - Regenerate man1 SYNOPSIS/OPTIONS and rewrite the hand-written parts. - docs/1.0.md and tools-migration(7) list each user-visible change. - Port the integration tests to the new verbs and merge related checks so the file seeds less: ~12s on SQLite, ~60s on Postgres. 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>
`bm ls /` was the only caller of project_mount_listing, and the mount listing was the only caller of other_workspace_slugs. Both go. The routing tests that read the mount set now read addressable_projects directly; the three tests of the listing itself are deleted. 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 routing comments explained their rules by what `ls /` advertised and by the mount view and posix verbs, all of which are gone. They now name what exists: the addressable project set and the prefixes tool output emits. Comments and docstrings only; the ASTs are unchanged. 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>
`bm cat <file>.txt --lines` and `bm head` on an indexed text file failed: bm_read refused every slice of a non-Markdown file, and the literary-analysis skill reads its source text a range at a time. - bm_read now serves start_line/end_line for text files with the same coordinates and continuation fields a note slice carries. section and max_tokens stay note-only; a line range on a binary file is refused. - find --meta splits a symbol predicate at its first operator character instead of a regex over '<', '>' and '=', which CodeQL read as an HTML comment filter. Same grammar, same refusals. - The literary-analysis skill pages the chapter-cast search until has_more is false, and no longer says grep has no line context. 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>
- bm tree strips the root in the spelling bm_find answers in: a path that
starts with the project's name comes back under its permalink
('my-research/specs'), so the tree nested the project segment under
the root for 'My Research/specs'.
- An empty document sliced as 1-0 of 0. The shared slicer now returns
the empty range 0-0 of 0 for notes and text files alike, and the line
formatter numbers no lines for it.
- bm_read renders the API's 404 for an indexed file whose bytes are gone
as a NOT_FOUND refusal in the requested format. The missing-file
integration test asserted the old UNSUPPORTED refusal of any line read
on a non-note; a line read on a text file is now supported, so the
refusal it reports is the missing content.
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>
bm grep answers which notes contain the pattern, so it passes items=["note"] to bm_search instead of relying on the search's default item kinds. A regression test pins one row per note when the note's observation and relation match too, with and without -C. 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>
Comments in the CLI, the manual generator and the redaction module still named retired MCP tools (write_note, delete_note, create_memory_project, basic_memory_diagnostics). They now name the bm_* surface or say what the code does. Comments and docstrings only; the ASTs are unchanged. 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 bm-setup skill told the agent to run `tool search --type ... --status
... --project-id`, flags removed in 1.0. It now runs the open-decision
query the SessionStart hook actually runs (--note-types decision
--filters '{"status": "open"}' --page-size 5), checks the seeded schemas
with --note-types schema, and passes ids through --project.
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>
With the posix functions and search_notes gone, tests/implementation_mcp.py bound no implementations. The prompt-routing test that used its server now uses the production mcp_server fixture, and test_v1_registration lists the retired tool names it checks explicitly. 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>
|
@codex review |
80b0e41 to
647a0aa
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". |
## 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>
Summary
The shell read verbs ran on
mcp/tools/posix_tools.py, the pre-1.0 MCP tool code. This PR rebuilds them as thin commands over the publishedbm_*tools, the same waybm toolruns them (call_tool/run_toolwithvalidate_call), and removesbm tail:bm cat,bm headbm_readbm grepbm_search(modetext;hybridwith--semantic)bm lsbm_listbm find,bm treebm_find(--metapredicates becomefilters)bm tailbm tool project recentSource changes:
posix_tools.pyis deleted, along with its__init__re-exports and the unusedmanfunction. The--metapredicate parser moves tocli/commands/find_predicates.py.cli/commands/posix.pygoes from about 1000 lines to about 400.cli/commands/output.py;bm inspectkeeps its output-mode helpers.project_mount_listing(mcp/directory_listing.py) andother_workspace_slugs(mcp/project_context.py) are deleted;bm ls /was their only caller. Cloud does not import either (GitHub code search plus a local checkout grep).run_toolnow delegates to a newcall_tool, which returns the tool result sotreecan draw it.bm tail, its man1 page and its references are removed. The benchmark seed guard (benchmarks/.../driver.py) now callsbm tool project recent --items note.just man-regen, and the hand-written DESCRIPTION and EXAMPLES are rewritten for the new behavior.skills/memory-literary-analysisis updated for the new flags and the 100-row page cap.User-visible changes (release notes)
These are also listed in
docs/1.0.mdandtools-migration(7).bm tailis removed. Usebm tool project recent.--plain, the Rich tables, and switching to JSON when piped are removed from cat/head/grep/ls/find/tree. Output is the tool's text result, or--json.--project-id UUIDis removed; use--project UUID.--project, the configured default project is used. Before, a multi-project config refused unqualified input.<project>/pathprefix no longer selects the project by itself. Pass--project <project>; a prefix naming a different project than--projectis refused.bm ls /lists the default project's root instead of listing projects.bm project listlists projects.bm cat --linesalways counts the whole file, frontmatter included.--frontmatter/--no-frontmatteronly affects whole-note reads. Before,--no-frontmatterranges counted body lines.bm catslices print numbered lines under aLines N-M of Theader. A whole-note read prints the note.bm headloses--frontmatter/--no-frontmatterand prints numbered lines.bm grep -C/--context-linesbecomes-C/--lines.bm grep ""and-Cwith--semanticnow followbm_search(neither is refused by the CLI).bm grep -Cfollowsbm_search's line matching: notes with no matching line are dropped andtotal_is_exactis false.bm grep --path,bm ls --glob,bm ls --sort.--page-sizeis 1-100 on every verb (ls/find/tree allowed up to 200).treedefaults to 100.bm find --fieldsprintsbm_find's text result instead of a table or tab-separated lines.Error: --page: Input should be greater than or equal to 1.Tests
test-int/cli/test_posix_commands_integration.py(7 tests): real-stack runs of every verb, seeded withbm tool write. Related checks share a test so each seeding is paid once.tests/cli/test_cli_posix_argument_errors.py: refusals before any I/O (--linesparsing, tool validation,bm_find/bm_readup-front refusals, the full--metagrammar refusal set, andbm tailbeing gone).tests/cli/test_find_predicates.py: the grammar translations (moved fromtests/mcp/test_find_meta_predicates.py).tests/cli/test_posix_tree.py: hierarchy rebuilding forbm tree.Removed:
tests/cli/test_cli_posix_verbs.pyandtests/cli/test_cli_line_scanning.py. They mocked the deleted functions to test the deleted renderers.tests/mcp/test_line_scanning.py: the three posix-grep-only tests (candidate pagination with zero matches, missingexternal_id, grep option validation). The rest now usesbm_read/bm_search.tests/mcp/test_tool_posix_man.py.tests/mcp/test_project_path_routing.py; the routing tests that read the mount set now readaddressable_projects.ls /mount view, and theBASIC_MEMORY_MCP_PROJECTenv-constraint CLI tests. These were posix-routing behaviors the CLI no longer has.Other updates:
tests/mcp/test_project_path_routing.pycallsproject_mount_listingandresolve_project_path_routedirectly instead of the deletedls/grep.tests/mcp/test_tool_contracts.py,tests/implementation_mcp.py,tests/cli/test_man_command.pyandtests/test_man_pages.pydrop the deleted names. The--json | --plainSYNOPSIS grouping is now pinned on a fake command, since no man1 verb has--plain.test-int/cli/test_find_note_type_integration.pydrops--plain.Review follow-ups
bm_readservesstart_line/end_linefor text files such as an indexed.txt, with the same coordinates and continuation fields as a note slice.sectionandmax_tokensremain note-only, and a line range on a binary file is refused. An indexed file whose bytes are missing returns a NOT_FOUND refusal in the requested format.bm treestrips the root in the permalink spellingbm_findanswers in, so'My Research/specs'no longer nests the project segment.bm greppassesitems=["note"]explicitly.find --metasymbol split is a plain scan instead of a regex. CodeQL flagged the regex as HTML filtering; that was a false positive, and the grammar is unchanged.has_moreis false, and the bm-setup skill's smoke test uses the 1.0 search flags and the hook's actual open-decision query.tests/implementation_mcp.pyis deleted because it no longer bound any implementations.test_v1_registrationlists the retired names explicitly, and the prompt-routing test usesmcp_server.Verification
just test-unit-sqlite: 8544 passed, 81 skipped (run after every change in this PR).just test-int-sqlite: about 12s of test time.just test-int-postgres: about 60s of test time (77s wall including the container). Before this PR the file had 72 tests and took 88s on SQLite uncontended.ruff,ty(apart from the pre-existing pymilvus imports),just package-check-skills, and the benchmarks driver tests pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01CMYVL1x91B8kJxy7nn2GU8