fix(mcp): replace edit_diagram 30s time gate with content comparison#890
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The 30s wall-clock gate rejected slow-but-correct clients (DayuanJiang#885). Instead of a timeout, remember the exact state-store XML the model last saw (get_diagram / create_new_diagram / edit_diagram / page CRUD) and reject edit_diagram only when the live browser state differs - i.e. the user made edits the model hasn't seen yet. Slow reasoning no longer trips the gate, while unseen manual edits still do.
…ased gate The 'You MUST call get_diagram BEFORE this tool' requirement and the 'Skipping get_diagram WILL cause user's changes to be LOST' warning no longer match server behavior: a stale edit is rejected with no side effects, never silently applied. Describe the freshness check instead, and direct get_diagram usage at its real purpose - learning the current diagram content when the model doesn't already know it.
draw.io re-serialises the document when pushing state back (attribute order, pretty-printing, regenerated diagram ids, viewport attributes, mxfile host), so byte comparison could flag an unchanged diagram as stale. Fingerprint what a user can actually change instead - page set, page names, and each page's root cell tree with sorted attributes - keeping byte equality as the fast path. A bare mxGraphModel now also fingerprints identically to its single-page mxfile wrapping.
|
Thanks for the PR, your diagnosis is spot on. However, the solution may run into trouble in the following scenario: In the last step, the agent should have called get_diagram before editing, since the user had moved the leg position. But it didn't, because it had already used get_diagram earlier in the conversation. I thought of a better way to solve this: since users rarely change the diagram themselves, the agent can just call get_diagram directly by default. I'd also add a mechanism to track whether the user changed the diagram. If they did, get_diagram will fail with a message: "please get_diagram before edit". |
A bare <mxGraphModel> pushed by the embed/sync path carries no page name, so normalizeToMxfile invents "Page-1" — falsely reading any custom page name as a content change and re-triggering the stale rejection on every edit. When either side of the gate comparison is a bare mxGraphModel, fingerprint cell trees only; full-mxfile comparisons still detect renames.
|
@DayuanJiang Thanks for the concrete scenario and the stronger design. Tracking actual diagram-content changes is much more robust than a conversation-level gate, and I appreciate you carrying it through to the merged implementation. |
Summary
get_diagramsnapshot after 30 seconds (the wall-clock gate rejected slow-reasoning clients even when they followed the documented workflow — MCP Tool Error Response: Error: You must call get_diagram first before edit_diagram. #885)lastSeenXml, set byget_diagram/create_new_diagram/edit_diagram/ page CRUD) and rejectsedit_diagramonly when the live browser state actually differs — i.e. the user made manual edits the model hasn't seen yetRelated to #885
Validation
npm test: 52/52 passed (45 existing + 4 new unit tests for the gate + wiring tests)edit_diagramwithoutget_diagram12s after create (browser had loaded and pushed autosave) → allowed, no false positive/api/statelike the browser autosave path), thenedit_diagramwithoutget_diagram→ rejected as staleget_diagram, thenedit_diagram→ allowed, recovery worksget_diagram, wait 35s (> old 30s limit),edit_diagram→ allowed, MCP Tool Error Response: Error: You must call get_diagram first before edit_diagram. #885 scenario fixedNotes
Built on top of @NgoQuocViet2001's original diagnosis in this PR: the 30s wall-clock gate was the root cause of #885. The follow-up commit keeps that fix while restoring the ability to detect mid-session manual browser edits, which the one-time sentinel check gave up.