Repository navigation
fix(mcp): correct manual tool results and cover deferred writes - #1864
Merged
Merged
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 #1843 Signed-off-by: TPSJASPER <592827953@qq.com> Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
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. |
Signed-off-by: phernandez <paul@basicmachines.co>
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. Builds on contributor PR #1860, cherry-picked from
TPSJASPER/basic-memoryatd4980fa3a3975c2b73db39125625a29749f395d0, 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
bm_contextafter materialization, preserving their observation/category/relation checks.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_contextafter 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.