fix(mcp): edit_note reports a refused write as an error

[sammywachtel]

Review copy. On the personal fork for review before it goes to basicmachines-co/basic-memory. Merging it here is not the goal.

Proposed upstream title: fix(mcp): edit_note reports a refused write as an error. The body below is the draft upstream description, with upstream references written out so nothing on this fork links or closes anything.

This fork's main is behind upstream, so the Files tab here also shows 25 upstream commits. The change under review is the one commit on top: d4ef89e

Why

When two clients append to the same note at the same moment, the engine sometimes has to turn one of them away: the db_version compare-and-set finds that the other edit landed first, and the API answers 409 with "The note was modified concurrently. Reload the latest content and retry." That check works as designed. Every refused append is missing from the note, and every accepted one is present.

The problem is how edit_note reports the refusal. It catches the ToolError and returns the "Edit Failed" text as an ordinary result, so the MCP result has isError false. A client that checks the error flag, which is what most programmatic clients do, counts the refused append as written and never retries it. The text says "Edit Failed"; the flag says success.

This is the same defect basicmachines-co/basic-memory PR 1641 fixed for delete_note and move_note ("A returned 'Delete Failed' string reads as success to MCP clients; raising makes the result an error (isError)"). edit_note has the same shape in its final failure branch.

Evidence

Two in-process MCP clients, 40 appends each to one note, SQLite, current main:

appends reported as written but absent from the note: ['A-29', 'B-0', 'B-22']

Each of those results had is_error=False and the text:

# Edit Failed

Error editing note '...': The note was modified concurrently. Reload the latest content and retry.

Across several runs, 3 or 4 of the 80 appends were refused this way, and none of the refusals had isError set. With this change, the refused appends (3 and 4 in two runs) all come back with isError true, and none of them is in the note.

I only saw this on SQLite. On Postgres, lock_accepted_note_content_for_entity_mutation takes a row lock (SELECT ... FOR UPDATE) that serializes the two edits, so I would not expect this race to produce refusals there. The reporting bug is the same on both backends: any edit the API refuses, for any reason, came back as a success.

What changed

• edit_note: an edit that fails or is refused now raises ToolError, so the MCP result is an error (isError). The message is the same guidance text as before, or the structured payload in JSON mode. A small _raise_edit_failure helper mirrors _raise_delete_failure and _raise_move_failure from basicmachines-co/basic-memory PR 1641.
• The unresolved-project-route stop (UNRESOLVED_PROJECT_ROUTE) in the same except handler raises the same way. Nothing was edited or created there either.
• bm tool edit-note: catches the ToolError, prints the payload's error field, and exits 1, the same handling basicmachines-co/basic-memory PR 1641 added to delete-note. Without it the command printed Error during edit_note: {...raw JSON...}.
• The edit_note docstring lists ToolError under Raises.

Testing

• just fix, just format, just typecheck: clean. (just typecheck needs the milvus extra installed, as CI does with uv pip install -e ".[dev,milvus,pdf]"; without it, ty reports 4 unresolved pymilvus imports in files this PR does not touch.)
• Unit modules (tests/mcp/test_tool_edit_note.py, test_tool_json_output_modes.py, test_tool_contracts.py, test_model_facing_call_examples.py, test_workspace_permalink_resolution.py, test_tool_telemetry.py, test_wiki_after_api_writes.py, test_tool_write_note.py, tests/cli/test_cli_tool_json_output.py, and the man-page tests): 283 passed. One failure and one error in this combined run, test_man_command.py::test_man_install_defaults_to_local_share_man and test_man_resources.py::test_unknown_pages_point_at_the_index ("fixture 'app' not found"). Both also occur on unchanged main with the same module set, and both pass when the man-page modules run alone.
• Integration modules (test-int/mcp/test_edit_note_integration.py, test_concurrent_write_integration.py, test_locked_note_results.py, test_output_format_json_integration.py, test_write_note_integration.py, test_move_note_integration.py, test_param_aliases_integration.py, test_default_project_mode_integration.py, test_long_relation_type_integration.py, test_project_state_sync_integration.py, test-int/cli/test_cli_tool_edit_note_integration.py, test-int/cli/test_routing_integration.py, test-int/bughunt_fixes/test_move_note_edge_cases.py): 192 passed, SQLite.
• Not run: the Postgres suites.

New tests, each of which fails on main:

• test-int/mcp/test_edit_note_integration.py::test_edit_note_refused_concurrent_write_is_an_error (text and JSON): makes NoteContentRepository.accept_write raise NoteContentVersionConflict, so the real 409 path runs through the API, then calls edit_note through an MCP client with raise_on_error=False. It asserts is_error is True, that the text names the concurrent modification, and that the file is unchanged. On main: assert False is True for both formats.
• test-int/mcp/test_concurrent_write_integration.py::test_concurrent_edit_append_reports_every_refusal: two MCP clients append 40 lines each at once. It asserts that every append reported as written is in the note and that no refused append is. On main: appends reported as written but absent from the note: ['A-29', 'B-0', 'B-22']. Where the backend refuses nothing, the test still passes, because both assertions hold trivially.
• tests/cli/test_cli_tool_json_output.py::test_edit_note_reports_a_tool_error_payload_and_exits_nonzero: on main the CLI printed Error during edit_note: {"title": null, ...}.

Updated tests: 12 unit tests in tests/mcp/test_tool_edit_note.py expected the failure text returned as a success. They now expect a ToolError with the same message, or the same JSON payload. 5 integration tests in test-int/mcp/test_edit_note_integration.py now call with raise_on_error=False and assert is_error is True. This is the same update basicmachines-co/basic-memory PR 1641 made for delete and move.

Risks / follow-ups

• Callers that parsed "Edit Failed" out of a successful result now receive an error with that same text instead. That's the intended change.
• The generic except also covers a failure after the API call succeeded (for example while formatting the summary). That path reported "Edit Failed" before this change and is now an error too. I found no way to reach it in practice, but a caller that retries every error could repeat an append that did land.
• Unchanged in this PR: the tool's own pre-mutation refusals still return their payloads as ordinary results: AMBIGUOUS_IDENTIFIER (workspace-qualified plain identifier), CROSS_PROJECT_ENTITY (identifier resolved to another project), and SECURITY_VALIDATION_ERROR (auto-create path outside the project). This matches basicmachines-co/basic-memory PR 1641 leaving the move tool's input-check failures as they were.
• Related: the checksum-guarded edit proposed in feat(mcp): expose and document checksum-guarded edit_note basicmachines-co/basic-memory#1552 would send its stale-revision conflicts through this same branch. With this change, those conflicts reach clients as errors.
• write_note's NOTE_REVISION_CONFLICT and NOTE_ALREADY_EXISTS remain structured non-error results, by design. This PR does not touch them.

[sammywachtel]

Opened upstream as basicmachines-co#1662; this review copy is no longer needed. The branch stays, because the upstream PR is opened from it.