Stop pretty-printing MCP tool results (#2350) - #2355
Conversation
|
Reviewed. This is a well-scoped change — flips Checks I ran to try to break the stated scope claims:
No findings to flag. Nice, honest PR description too (the "token savings are smaller than byte savings" caveat is the kind of detail that's easy to oversell and wasn't). |
|
CI caught a gap in my own pre-change check, and the fix is worth describing because the first instinct would have been wrong. What I missed. I verified "nothing depends on the layout" by grepping for tests asserting on serialized output containing newlines. Eighteen assertions depended on the space after the colon instead — Why I did not just retype them compact. Every one of those assertions reads as a claim about content: this field serialized with this value, this enum came out as its string name rather than its ordinal, this null stayed null. They were written as claims about formatting. Flipping So Still substring assertions rather than a full parse, on purpose — they pin one field's serialization without pinning the shape of the envelope around it. Four format-coupled assertions are deliberately left alone: the exported |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public static readonly JsonSerializerOptions JsonOptions = new() | ||
| { | ||
| WriteIndented = true, | ||
| WriteIndented = false, |
There was a problem hiding this comment.
Test-coverage gap, not a bug: this flip (and the matching one in DarlingFleetReader.cs:50) has no Darling-side pin.
Lite gets a regression test for exactly this boundary — Lite.Tests/McpOutputCompactionTests.cs asserts McpHelpers.JsonOptions.WriteIndented == false and, in the other direction, that ServerManager/ProfileManager/ScheduleManager source still contains WriteIndented = true. Darling has no counterpart for DarlingAgReader/DarlingFleetReader, and the existing tests that were touched here (DarlingAgReaderTests, DarlingFleetReaderTests) don't fill the gap: they now route through the new JsonAssert.Contains/DoesNotContain, which strips whitespace outside string literals from both sides before comparing. That makes them pass identically whether WriteIndented is true or false here — so nothing in the Darling suite would fail if this line were reverted to true, and nothing would catch an accidental compaction of the Viewer's own config writers (ViewerServerStore, ViewerProfileStore, ViewerAlertStateService, ViewerAppSettings, ViewerPreferences, all still WriteIndented = true).
Worth adding a Darling-side analog of McpOutputCompactionTests (direct assert on DarlingAgReader.JsonOptions.WriteIndented / DarlingFleetReader.JsonOptions.WriteIndented, plus a source pin that the Viewer writers stay indented) to close the same regression risk this PR just closed for Lite.
|
Reviewed. This is a well-scoped, well-audited change — no T-SQL touched, so the collector-style conventions don't apply here. The scoping claims in the PR description check out against the actual tree:
One gap flagged inline: Lite gets a regression test pinning the compact/indented boundary in both directions ( |
The only consumer of an MCP tool result is a language model, and indentation buys a model nothing (#2350). It was one property on one shared object in Common, so both SKUs move together: 78 call sites across the Darling and Lite MCP surfaces already route through McpHelpers.JsonOptions. The two readers that keep their own options for the /api/* twins get the same treatment, so the web endpoint and the tool still serialize an identical shape. The saving is payload-shaped and should not be oversold: 23% of the BYTES on a 15-field record array, 36% on a narrow one, and the TOKEN saving is smaller than either because BPE tokenizers pack runs of spaces efficiently. It costs nothing, which is the argument. Scope was the whole risk here, not mechanism. The config files people open and hand-edit keep indenting -- ServerManager, ProfileManager and ScheduleManager carry their own options and are untouched -- because flipping this on a file writer would turn servers.json into one unreadable line, which nobody notices until an awkward moment. The new test pins both directions: the tool output carries no layout whitespace, and those three writers still say WriteIndented = true. Verified nothing depended on the layout first: no test asserts on serialized MCP output containing newlines, the 24 files that parse tool JSON use a parser, and the tests that touch this options object assert field NAMES rather than shape. Found while reviewing #2286, which benchmarked a third-party wire format against our indented output -- so some of the win it reported was a serializer flag. Credit to @blackwell-systems for the measurement that surfaced it; this half carries no dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI caught what my pre-change check missed. I grepped for tests asserting on serialized output containing NEWLINES and concluded nothing depended on the layout. Eighteen assertions depended on the space after the colon instead -- "severity": "Critical" only exists under WriteIndented -- and four test classes failed without a single thing they were testing having changed. Those assertions read as claims about content (this field serialized with this value, an enum as its string name rather than its ordinal, a null that stayed null) but were written as claims about formatting. So the fix is to make them mean what they looked like they meant, not to retype the literals in compact form -- which would leave the same trap armed for whoever changes the formatting next. JsonAssert.Contains/DoesNotContain normalize both sides by dropping whitespace BETWEEN tokens while preserving whitespace INSIDE strings, so "a": "b c" and "a":"b c" compare equal and the two-space value in "b c" survives. Escaping is tracked so a \" inside a string does not end it and a \\ before a quote does not escape it; get that wrong and the scan falls out of the string, starts stripping real spaces from values, and the assertion silently compares something else. Deliberately still substring assertions rather than a full parse: they check that one field serialized a particular way without pinning the shape of the envelope around it. Four format-coupled assertions elsewhere are deliberately untouched -- exported darling.json, the network config editor, a stored command result and a static options constant. Those are config files people read and a literal, none of them MCP output, and CI passed them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9b54b6b to
b304a86
Compare
| Assert.Contains("\"band\": \"Critical\"", json, StringComparison.Ordinal); | ||
| Assert.Contains("\"cpu_severity\": \"Critical\"", json, StringComparison.Ordinal); | ||
| Assert.Contains("\"threads_severity\": \"Unknown\"", json, StringComparison.Ordinal); | ||
| JsonAssert.Contains("\"band\": \"Critical\"", json); |
There was a problem hiding this comment.
Nice catch on the layout-vs-content conflation with JsonAssert, but the fix looks one-sided: JsonAssert.Contains/DoesNotContain strip whitespace from both the fragment and the actual JSON before comparing. That means every assertion in this file (and DarlingAgReaderTests.cs) now passes identically whether DarlingFleetReader.JsonOptions/DarlingAgReader.JsonOptions is indented or compact — they no longer provide any signal on the very property this PR flips (WriteIndented).
Contrast with Lite.Tests/McpOutputCompactionTests.cs, which pins the shared McpHelpers.JsonOptions.WriteIndented == false directly plus a "no layout whitespace at all" check. DarlingAgReader.JsonOptions and DarlingFleetReader.JsonOptions got the identical WriteIndented = false flip in this same PR but have no equivalent pin — someone flipping either back to true (e.g. "make the /api/* output readable") would break nothing here.
Worth adding a small Assert.False(DarlingFleetReader.JsonOptions.WriteIndented) / same for DarlingAgReader so the two Darling-only readers get the same regression coverage as the Lite/Common path.
Review summaryThis is a narrow, well-contained change (one property on the shared
One gap worth a look — left as an inline comment on No correctness, security, or performance concerns — this is JSON output formatting only, no new input handling or SQL surface. |
1 participant
Closes #2350.
One property on the shared
McpHelpers.JsonOptionsin Common — 78 call sites across both SKUs' MCP surfaces already route through it — plus the two readers (DarlingAgReader,DarlingFleetReader) that carry their own options so the/api/*endpoints and the MCP tools keep serializing an identical shape.The saving, stated honestly
Payload-shaped: 23% of the bytes on a 15-field record array, 36% on a narrow one. The token saving is smaller than the byte saving — BPE tokenizers pack runs of spaces efficiently, so anyone quoting these numbers as token savings will be wrong. The argument is that it costs nothing and compounds where it matters: tool results are the bulk of what fills an agent's context during a real incident, and the fleet-wide reads are the widest results we return.
Scope was the whole risk
Flipping
WriteIndentedon something that turns out to write a config file would makeservers.jsona single unreadable line — the kind of damage nobody notices until they open the file by hand at a bad moment. So I checked before changing:McpHelpers.JsonOptionsis MCP-output only. Of 78 usage files, exactly one sits outside anMcp/folder (McpPlanAnalysisFormatter, still an MCP formatter), and nothing writes a file with it.ServerManager,ProfileManagerandScheduleManagercarry their ownWriteIndented = trueand are untouched.Nothing depended on the layout
\nhits in the suites are an axis tick label and a hand-written JSON literal, both unrelated).[JsonPropertyName]— not layout.Tests
McpOutputCompactionTestspins the flag, the observable consequence (no\n, no\r, no indent run), that compaction changes layout only and not content (same parsed values, and genuinely smaller), and the boundary — those three config writers still indent, asserted structurally the way this repo pins invariants it cannot compile.Verification
Builds clean, 0 warnings. Suites target
net10.0-windowsso they cannot run on macOS; every assertion in the new test was exercised first in a throwawaynet10.0harness and passes.Provenance
Found while reviewing #2286, which proposed a third-party wire format and benchmarked it against our indented output — so part of the win it reported was a serializer flag rather than the format. Credit to @blackwell-systems for the measurement that surfaced this; this half carries no new dependency and stands on its own.