Repository navigation
fix(mcp): tool answers spell + < > & as themselves — the reviewer read += as \u002B= (Plugins#2804) - #6239
Conversation
… `+=` as `+=` MeshOperations wrote every tool answer (get, search, status envelopes) with the hub's serializer options, whose default encoder is HTML-safe. A PR diff therefore reached the internal reviewer as `\n+ runs += json.loads(...)` - the added-line marker and the operator spelled identically - and three review rounds on Plugins#2461 reported the file's only two `+=` lines as assignments, one as a BLOCKING finding that held the PR (Plugins#2804). The same spelling sits under rebutted blockers on core (#6181 `failures +=`, #6162 `names +=`, #6210 `/// <summary>` read as a stray end tag). ToolAnswerJson writes the answer through a Utf8JsonWriter with the relaxed encoder, keeping the hub's converters/naming - the answer parses to the same document, only the spelling changes. Pinned by AToolAnswerSpellsSourceCharactersAsThemselvesTest (negative control: with MeshOperations reverted, the Get case fails showing exactly the reviewer's `+=` input). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Three central documentation passages incorrectly show the old escaped output as literal source characters.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
This PR preserves source characters in MCP/tool JSON answers so reviewers and agents interpret diffs correctly.
Changes:
- Adds relaxed JSON serialization for tool responses.
- Migrates MeshOperations response sites to the new serializer.
- Adds regression coverage and documentation.
| File | Description |
|---|---|
ToolAnswerJson.cs |
Adds relaxed tool-answer serialization. |
MeshOperations.cs |
Applies it across tool responses. |
MeshOperations.RunTests.cs |
Applies it to test-run responses. |
AToolAnswerSpellsSourceCharactersAsThemselvesTest.cs |
Tests spelling and semantic equivalence. |
MeshPlugin.md |
Documents the response encoding contract. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results 8 files - 14 8 suites - 14 20m 54s ⏱️ - 28m 17s Results for commit 87e0ac1. ± Comparison against base commit c375173. This pull request removes 5635 and adds 7 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…prose The rationale in ToolAnswerJson, the test's incident note and the MeshPlugin doc showed the reviewer's input with literal `+`, which states the opposite of the defect: the authoring tool decoded the escapes on the way into the files (the same class of defect this PR fixes). The examples are now written as the literal escape text the model received. The first commit's message carries the same decoding; this message and the PR body are authoritative. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 3) 1 files - 3 1 suites - 3 45s ⏱️ - 5m 12s Results for commit 87e0ac1. ± Comparison against base commit c375173. This pull request removes 2602 and adds 410 tests. Note that renamed tests count towards both. |
Test Results (shard 4) 2 files - 2 2 suites - 2 1m 1s ⏱️ - 15m 54s Results for commit 87e0ac1. ± Comparison against base commit c375173. This pull request removes 3623 and adds 691 tests. Note that renamed tests count towards both. |
Test Results (shard 2) 1 files - 4 1 suites - 4 3m 53s ⏱️ -13s Results for commit 87e0ac1. ± Comparison against base commit c375173. This pull request removes 1167 and adds 1350 tests. Note that renamed tests count towards both. |
Test Results (shard 1) 1 files - 3 1 suites - 3 4m 3s ⏱️ - 1m 34s Results for commit 87e0ac1. ± Comparison against base commit c375173. This pull request removes 1818 and adds 1130 tests. Note that renamed tests count towards both. |
Test Results (shard 0) 1 files - 2 1 suites - 2 6m 56s ⏱️ -58s Results for commit 87e0ac1. ± Comparison against base commit c375173. This pull request removes 212 tests. |
Test Results (shard 5) 2 files ± 0 2 suites ±0 4m 12s ⏱️ - 4m 28s Results for commit 87e0ac1. ± Comparison against base commit c375173. This pull request removes 1118 and adds 1331 tests. Note that renamed tests count towards both. |

Refs Systemorph/MeshWeaver.Plugins#2804
The defect
The internal pull-request reviewer reads a PR's unified diff with one
getof itsHosting/TriageItem.MeshOperationswrote that answer (and every other tool answer) withhub.JsonSerializerOptions. Those options use System.Text.Json's HTML-safe default encoder, which spells+ < > & ' ```` and all non-ASCII as\uXXXX.The round's own stored tool result shows what the model received (thread
Hosting/Triage/_Thread/review-pull-request-systemorph-meshweave-929d, z-ai/glm-5.3), and the negative control below reproduces it byte for byte:Here the added-line marker and the operator are spelled the same way.
scripts/pr-supersede.pyate7c2abf7b/790e638e2has exactly two+=lines (78 and 126). Three separate review rounds on Plugins#2461 reported both of them as plain assignments, and two of those findings were blocking: comments 4116904325, 4117032726 and 4117265000.It is a pattern, not a one-off
I sampled the last ~700 review comments on core and Plugins and kept the blocking findings that were rebutted on the thread. Four of the seven false blockers misread a character this encoder rewrites:
failures +=read asfailures =names +=read as a replace/// <summary>read as a stray</summary>The fix
ToolAnswerJsonserializes through aUtf8JsonWriterwithJavaScriptEncoder.UnsafeRelaxedJsonEscaping. It keeps the hub's converters, naming and$type, so every answer parses to the same document as before and only the spelling changes. A tool answer is never embedded in HTML, so the relaxed encoder is the correct one here. Every answer site inMeshOperations(andRunTests) now uses it. Three call sites are left as they were because they are not answers: a property-name lookup, a content round-trip validation, and the export manifest file.Evidence
AToolAnswerSpellsSourceCharactersAsThemselvesTest(Graph.Test), 2/2 passing:get, and asserts thatruns += …,+ failures += …,/// <summary>a && b …appear literally and that no\u002B/\u003C/\u0026does. It also parses the answer back and checks it equals the stored value.\u002B=, so the test cannot pass vacuously.JsonNode.DeepEquals(relaxed, hub)is true.MeshOperations.csreverted, the Get case FAILS, and its DIAG shows"description":"@@ -0,0 \u002B1,3 @@\n\u002B runs \u002B= json.loads(out or \u0022{}\u0022)…". That is the reviewer's input.Memex.Portal.Shared.Testpassed (64/64), as did the Graph MeshOperations tests (10/10).dotnet build -c Release -warnaserroris clean, one project at a time, for: Mesh.Operations, Graph.Test, Memex.Portal.Shared.Test, PluginTester and Documentation.Rollout
This reaches the reviewer only once the control instance (memex.systemorph.com) runs an image with this commit. Agent rounds and MCP both go through
MeshOperations.Get, inMeshPlugin.GetandMcpMeshPlugin.Getrespectively. No per-node hub needs recycling, becauseMeshOperationsis constructed per plugin and holds no cached answers.What I did not establish
+=. The case rests on the stored tool result plus three independent misreads of exactly the escaped lines.|, AccessControlPipeline restores a delivery's AccessContext only when Roles is non-empty — an API token with no claims reaches a per-node hub unclamped #2976 surface claims) do not involve an escaped character. Those look like ordinary reviewer error, and this change does not address them.Pairs-with: none — adds a public type and changes answer spelling only; no public member removed.
Implementers: none — no interface member added.
Mirror-sync: none — no catalog key added or changed.
🤖 Generated with Claude Code