Skip to content

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

Open
sammywachtel wants to merge 1 commit into
basicmachines-co:mainfrom
sammywachtel:fix/edit-note-refusal-is-an-error
Open

sammywachtel wants to merge 1 commit into
basicmachines-co:mainfrom
sammywachtel:fix/edit-note-refusal-is-an-error

Conversation

@sammywachtel

@sammywachtel sammywachtel commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

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 #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 fix(mcp): report failed delete_note and move_note as tool errors #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 fix(mcp): report failed delete_note and move_note as tool errors #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 #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 fix(mcp): report failed delete_note and move_note as tool errors #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 #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.

When the engine refuses an edit, for example with a 409 because a
concurrent edit advanced the note's db_version first, edit_note caught
the ToolError and returned the "Edit Failed" text as an ordinary result.
MCP clients that check isError read the refusal as success, so a refused
append was counted as written and never retried.

Raise ToolError instead, as delete_note and move_note already do: the
message is the same guidance text, or the structured payload in JSON
mode. The unresolved-project-route stop in the same handler raises too.
The CLI edit-note command reports the tool error's error field and
exits 1, matching delete-note.

Signed-off-by: sammywachtel <subp@wachtel.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant