Repository navigation
Conversation
Report only the accepted revision from bm_write, bm_edit and bm_move: the graph counts came from the pre-index response and always read zero. Make bm_search's line view honest. A one-note search says when no line matches, ranked candidates without a literal matching line are dropped, and filtered line totals are marked inexact while the next page stays reachable. bm_context pattern searches prefer notes; a pattern that names no note still selects relation or observation permalinks directly, and later note pages do not fall back to graph rows. Return the first nonblank line when bm_read's token budget is smaller than it, so a tiny budget cannot produce a whitespace-only page. Point bm_schema guidance at the bm_* tools and at bm reindex. Fixes basicmachines-co#1843 Signed-off-by: TPSJASPER <592827953@qq.com>
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
|
Thanks, @TPSJASPER, for finding and fixing these manual-tool regressions, and for the regression coverage! We’ve cherry-picked your commit into #1864, preserving your authorship and sign-off. That PR carries your fixes forward and adds integration coverage using real deferred materialization, files, indexing, and embeddings to exercise production behavior alongside our inline tests. Closing this PR in favor of #1864. Thank you for the contribution! |
phernandez
added a commit
that referenced
this pull request
Oct 25, 2026
## Why Fixes #1843. Builds on contributor PR #1860, cherry-picked from `TPSJASPER/basic-memory` at `d4980fa3a3975c2b73db39125625a29749f395d0`, preserving the original author and sign-off. The manual examples exposed misleading write counts, ambiguous empty line searches, graph rows appearing as primary note results, whitespace-only reads under tiny budgets, and guidance naming obsolete tools. Existing tests missed the write-response problem because test mode indexes writes inline while production accepts the note and queues materialization. ## What Changed - Carry forward the contributor's six fixes and manual updates: accepted write identity/checksum results, explicit no-line-match output, filtering of unmatched line candidates with honest pagination, note-first graph patterns with graph-address compatibility, nonblank tiny-budget reads, and current schema guidance. - Add eight integration cases through the real MCP client/server and ASGI API with file-backed SQLite or Postgres, production deferred materialization, actual graph/index updates, and real FastEmbed inference. - Update existing write/edit integration tests to read graph counts through `bm_context` after materialization, preserving their observation/category/relation checks. - Replace the new mocked search-candidate tests from #1860 with real vector/hybrid integration cases, covering both matching and unmatched candidates and continuation through an empty filtered page. ## Implementation Details The integration context selects the production environment inside the test call and opts out of the deterministic test embedder. A barrier occupies the real single-worker materialization queue, so the tests can assert that accepted content is readable while its file write is still queued. Releasing the barrier and awaiting the existing materialization/background drains proves eventual file and graph convergence without polling sleeps or replaced application functions. The new flows cover create/edit/move before and after materialization in text and JSON, literal line filtering over real semantic candidates, one-note misses, note-only graph pagination, nonblank read continuations at tiny budgets, and public schema guidance. Existing inline tests continue to provide fast focused coverage. ## Testing - `uv run pytest test-int/mcp/test_deferred_manual_contracts.py -q -x -o addopts=` — 8 passed on SQLite. - `BASIC_MEMORY_TEST_POSTGRES=1 uv run pytest test-int/mcp/test_deferred_manual_contracts.py -q -x -o addopts=` — 8 passed on real Docker Postgres. - `uv run pytest test-int/mcp/test_build_context_underscore.py -q -o addopts=` — 2 passed, preserving explicit relation/observation address patterns. - `uv run pytest tests/mcp/test_note_writes.py tests/mcp/test_tool_bm_manual_regressions.py tests/mcp/test_tool_bm_write.py tests/mcp/test_tool_bm_search.py tests/mcp/test_tool_bm_context.py tests/mcp/test_tool_bm_read.py tests/mcp/test_tool_schema.py tests/mcp/test_man_resources.py tests/markdown/test_sections.py tests/markdown/test_token_budget_nonempty.py tests/services/test_context_glob_primary_notes.py tests/services/test_context_service.py tests/test_man_pages.py tests/cli/test_man_command.py -q -o addopts=` — 332 passed. - `uv run pytest test-int/mcp/test_write_note_integration.py test-int/mcp/test_edit_note_integration.py -q -x -o addopts=` — 48 passed on SQLite. - `BASIC_MEMORY_TEST_POSTGRES=1 uv run pytest test-int/mcp/test_write_note_integration.py::test_write_note_complex_content_with_observations_relations test-int/mcp/test_edit_note_integration.py::test_edit_note_with_observations_and_relations test-int/mcp/test_edit_note_integration.py::test_edit_note_special_characters_in_content -q -x -o addopts=` — 3 passed on Postgres. - `just fast-check` — lint, format and full typecheck passed. - `just doctor` — sandboxed write/index/search passed. - `git diff --check` — passed. ## Risks / Follow-ups Write-result JSON drops the three graph-count fields; callers should use `bm_context` after indexing when they need graph state. The new integration tests require the real embedding model and take roughly a minute on SQLite and two minutes on local Postgres. They use a real queue barrier for deterministic scheduling and close both worker pools during teardown. Full CI and current-head Codex review are checked on this PR. --------- Signed-off-by: TPSJASPER <592827953@qq.com> Signed-off-by: phernandez <paul@basicmachines.co> Co-authored-by: TPSJASPER <592827953@qq.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.
Why
Fixes #1843
Six defects found while verifying the
bm_*manual examples. They misreport graph counts, make filtered searches look like matches, page the wrong row kinds, return empty reads, and point guidance at legacy tool names.What changed
bm_write,bm_editandbm_movereport the accepted path, title, permalink and checksum. The observations/relations counts came from the pre-index response and always read zero, so they are gone; read the graph withbm_context.bm_search(identifier=...)now showsNo matching lines.in text and keepsmatch_count: 0with empty windows in JSON.linessearch. Ranked candidates whose accepted Markdown has no literal matching line are dropped in every mode. An empty filtered page keeps its next-page call, andtotal_is_exactis false becausetotalcounts ranked candidates, not matched rows.bm_contextpatterns. Wildcard searches select notes first, so observations and relations are no longer primary rows. A pattern that names no note still addresses relation/observation permalinks directly, which keeps relation-shaped patterns such ascomponents/*/part_of/*workflow*working; later pages of a note pattern do not fall back to graph rows.bm_read(max_tokens=...)keeps the first nonblank line and its leading whitespace even when it exceeds the approximate budget, so a small budget cannot return a whitespace-only page. Line coordinates and the continuation line stay usable.bm_schema(command=...),bm_write,bm_search,bm_listandbm reindex --searchinstead of the legacy tool names.The
bm-write,bm-search,bm-contextandbm-readmanual pages document the resulting behavior.Validation
test-int/mcp/test_build_context_underscore.pypassed, covering the relation/observation wildcard patterns.ruff check, changed-fileruff format --check,ty checkfor the changed files, andgit diff --checkpassed.doctor --localcompleted sandboxed write, index and search in an isolated config directory.The full-repository
ty checkstill reports the same 8 environment diagnostics as the unmodified baseline (optionalpymilvus/uvloop, Windowsresourcemembers); no ignores were added. The full test suite and Postgres tests were not run. The implementation was also replayed againstorigin/mainwithgit apply --check.Risks / rollback
The write result drops three JSON fields; callers that relied on the always-zero counts must read the graph instead.
bm_contextkeeps graph-row patterns only when no note matches, so mixed note/relation patterns resolve to notes. Revert the commit to roll back.