Repository navigation
Conversation
…ines-co#2065) Through the local ASGI transport an exception raised while serving the search reaches the advisory probe as itself, not as an HTTP 500. The probe caught only ToolError, so a sqlite-vec OperationalError on the search side failed bm_write after the note was already on disk, and a caller that retries can duplicate the note. The probe now also catches any other Exception, logs it with its traceback and returns no advisory. CancelledError is a BaseException and still propagates. The test raises the reported OperationalError from SearchService.search, so it travels through the search route as in the issue; it fails without the change. Assisted-by: Claude Code/claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Anton Dziatkovskii <194927794+tonydzi@users.noreply.github.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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 487e4d7d9c
ℹ️ 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".
| probe_log = logger.debug if probe_declined_by_server(probe_error) else logger.warning | ||
| probe_log(f"bm_write similar-note probe failed for {written.file_path}: {probe_error}") | ||
| return [] | ||
| except Exception as probe_error: |
There was a problem hiding this comment.
For the reported local-ASGI OperationalError this fallback is appropriate, but find_similar_notes() also performs response JSON/Pydantic validation, link resolution, and result collapsing; any defect in those paths now matches this handler and bm_write reports similar_notes: [], hiding the broken duplicate advisory from the caller on every create. Limit the handler to the known operational/transport failures (or normalize those at the ASGI boundary) so schema and programming errors still fail fast; the repository explicitly forbids broad exception swallowing.
AGENTS.md reference: AGENTS.md:L131-L134
Useful? React with 👍 / 👎.
Fixes the protocol half of #2065:
bm_writereports an error although the note was written.Why it failed
The similar-notes advisory runs after the note is on disk and is meant never to fail the call.
_similar_notescaught onlyToolError. With the local ASGI transport, an exception raised while serving the search is not turned into an HTTP 500: it reaches the client as itself.call_queryre-raises anything that is not anHTTPStatusError,TransportErrororTimeoutError, andsearch_error_boundarydoes not mapOperationalError. So the vec0OperationalErrorfrom the report went past the probe and failedbm_write. As the second comment notes, a caller that retries on that error can end up with a duplicate note.What changes
_similar_notesalso catches any otherException, logs it at warning with its traceback (logger.opt(exception=...)), and returns no advisory.CancelledErroris aBaseException, so cancellation still propagates. A declined probe (HTTP 400) keeps its current debug-level handling.test_a_database_error_in_the_probe_does_not_fail_the_writeraises the reportedOperationalErrorfromSearchService.search, so it travels through the search route and the ASGI transport the way the report describes. It checks the service was called once, the write returnscreatedwith no advisory, andbm_readreturns the body. It fails onmainwith the same error text as the issue, and passes with the change.What this does not fix
Why the KNN query hits a vec0 auxiliary-column constraint on these installs is still open. I could not reproduce it: the current schema (
project_id integer partition key) accepts the sameproject_id IN (...) AND rowid IN (...)query on sqlite-vec 0.1.6, 0.1.7, 0.1.8, 0.1.9 and 0.1.10a4. The exact error appears only whenproject_idis an auxiliary (+project_id) column, which no basic-memory schema I found has created. The output ofSELECT sql FROM sqlite_master WHERE name = 'search_vector_embeddings'from an affected database would settle it.Checks
tests/mcp/test_tool_bm_write.py: 40 passed.tests/mcp: 1281 passed (run before the final rewrite of the new test; the rewritten file passes 40 of 40).ruff checkandruff format --checkclean. Local env on Intel macOS withoutonnxruntime/fastembed(no wheel for that platform).This PR was written by Claude Code (an AI agent) for Anton Dziatkovskii. Four other models reviewed the diff before submission; from that review the test now raises from the server side instead of the HTTP client, and the warning carries the traceback.
Anton Dziatkovskii
🤖 Generated with Claude Code