Repository navigation
fix(mcp): edit_note reports a refused write as an error - #1662
Open
sammywachtel wants to merge 1 commit into
Open
sammywachtel wants to merge 1 commit into
sammywachtel wants to merge 1 commit into
Conversation
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>
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
When two clients append to the same note at the same moment, the engine sometimes has to turn one of them away: the
db_versioncompare-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_notereports the refusal. It catches theToolErrorand returns the "Edit Failed" text as an ordinary result, so the MCP result hasisErrorfalse. 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_noteandmove_note("A returned 'Delete Failed' string reads as success to MCP clients; raising makes the result an error (isError)").edit_notehas the same shape in its final failure branch.Evidence
Two in-process MCP clients, 40 appends each to one note, SQLite, current
main:Each of those results had
is_error=Falseand the text:Across several runs, 3 or 4 of the 80 appends were refused this way, and none of the refusals had
isErrorset. With this change, the refused appends (3 and 4 in two runs) all come back withisErrortrue, and none of them is in the note.I only saw this on SQLite. On Postgres,
lock_accepted_note_content_for_entity_mutationtakes 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 raisesToolError, 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_failurehelper mirrors_raise_delete_failureand_raise_move_failurefrom fix(mcp): report failed delete_note and move_note as tool errors #1641.UNRESOLVED_PROJECT_ROUTE) in the sameexcepthandler raises the same way. Nothing was edited or created there either.bm tool edit-note: catches theToolError, prints the payload'serrorfield, and exits 1, the same handling fix(mcp): report failed delete_note and move_note as tool errors #1641 added todelete-note. Without it the command printedError during edit_note: {...raw JSON...}.edit_notedocstring listsToolErrorunder Raises.Testing
just fix,just format,just typecheck: clean. (just typecheckneeds themilvusextra installed, as CI does withuv pip install -e ".[dev,milvus,pdf]"; without it,tyreports 4 unresolvedpymilvusimports in files this PR does not touch.)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_manandtest_man_resources.py::test_unknown_pages_point_at_the_index("fixture 'app' not found"). Both also occur on unchangedmainwith the same module set, and both pass when the man-page modules run alone.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.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): makesNoteContentRepository.accept_writeraiseNoteContentVersionConflict, so the real 409 path runs through the API, then callsedit_notethrough an MCP client withraise_on_error=False. It assertsis_error is True, that the text names the concurrent modification, and that the file is unchanged. Onmain:assert False is Truefor 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. Onmain: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: onmainthe CLI printedError during edit_note: {"title": null, ...}.Updated tests: 12 unit tests in
tests/mcp/test_tool_edit_note.pyexpected the failure text returned as a success. They now expect aToolErrorwith the same message, or the same JSON payload. 5 integration tests intest-int/mcp/test_edit_note_integration.pynow call withraise_on_error=Falseand assertis_error is True. This is the same update #1641 made for delete and move.Risks / follow-ups
exceptalso 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.AMBIGUOUS_IDENTIFIER(workspace-qualified plain identifier),CROSS_PROJECT_ENTITY(identifier resolved to another project), andSECURITY_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.write_note'sNOTE_REVISION_CONFLICTandNOTE_ALREADY_EXISTSremain structured non-error results, by design. This PR does not touch them.