feat(web): use the Ace JSON editor everywhere, and restore the raw-JSON toggle

[cliffhall]

Closes #2151

Puts the Ace editor from #2094 behind every place the web client types or displays JSON, and restores v1's raw-JSON toggle on tool forms.

What changed

A shared element, elements/JsonEditor. JsonObjectInput (#2094) carried the Ace integration inline, on an object-shaped contract SchemaJsonField cannot use. The Ace wiring — the module-scoped worker URL, the theme, the label/aria-describedby/aria-invalid wiring onto Ace's hidden textarea, and the coalescing of Ace's paired remove/insert change events — moves into JsonEditor, which is deliberately text in, text out.

That split is the design decision worth reviewing. The two editing contracts disagree about what an unparseable draft means — JsonObjectInput keeps the last valid object and says nothing to its parent (there is no Save button to gate), while SchemaJsonField reports undefined and blocks submission (#2020) — and no component that decides for them can serve both. So parsing, and the draft/value split that goes with it, stays with each caller; only the editing surface is shared.

JsonObjectInput is now a thin wrapper over it and behaves exactly as before.

The six surfaces:

#	Surface	Now
1	SchemaJsonField — the object/array/union fallback in SchemaForm	Ace. Inherited by all four SchemaForm consumers: Tools, Apps, and both elicitation panels.
2	Edit as JSON switch on SchemaForm	New. Edits the whole arguments object; honors #2020 draft validity and #2123 variant pruning.
3	ImportServerJsonPanel → File Contents	Ace.
4	ExperimentalFeaturesPanel → Request	Ace.
5	ContentViewer's JSON branch	Ace, read-only. Eleven consumers inherit it.
6	Edit and replay on a Protocol entry	New. Ace editor + Send/Cancel, dispatching through the existing replayProtocolRequest.

The decision the issue asked to be stated on the PR

Ace read-only replaces the CodeHighlight JSON path, and the Prism json grammar loader is dropped. Colorization was never the win — JSON already highlighted through the lazily-imported Prism grammar. What Ace adds is folding, line numbers and a gutter on a large payload. Keeping both would leave two highlighters for one language, differing in appearance and drifting apart, which is exactly what the issue flagged. Nothing else asks for json, so the loader entry is now dead code and is removed; CodeHighlight keeps markup / css / yaml / markdown and its tests were re-pointed at yaml.

Two cases deliberately stay on the plain renderer:

• wrap={false} callers (the server card's fixed-height, single-line, ellipsis-clipped box), as the issue requires.
• Untyped text that only looks like JSON. looksLikeJson tests the first character, which is enough to decide whether to try pretty-printing — a failed parse falls back and nothing is lost. It is not enough to decide to open an editor: a note beginning { would be presented as a document, in a JSON gutter, with the server's prose framed as malformed. A new isJsonDocument guards that path by actually parsing. A declared application/json is different — there the server said what it sent — so its broken payload does render as JSON.

The copy overlay is unchanged: ContentViewer overlays it above whichever renderer it picked, so copyable keeps working across the swap.

Things that needed care

• "Not edited" vs "edited to no params". The Edit-and-replay editor has to be able to say both "send these params" and "send none" — and neither can be undefined, which is what the plain Replay button passes to mean "unedited". A new ReplayParamsOverride (Record | null | undefined) makes the three states distinct; collapsing null into undefined would make an emptied editor silently re-send the original params.
• The raw-JSON block is held apart from invalidFields. Field names come straight out of a server's schema, so any sentinel name the form invented for the raw editor could collide with a real argument and clear a block the user cannot see.
• The switch is suppressed on a nested object's form. A per-field switch would offer to edit a fragment of the payload the outer switch already covers whole — and would nest one JSON editor inside another the moment both were on.
• Ace virtualizes its lines, so a read-only editor's text is not fully in the DOM. Assertions that used to read container.textContent now go through the editor instance (getAceText), and test/aceEditor.ts gains *ByLabel variants for a screen holding more than one editor.
• Ace cannot be typed into under happy-dom — userEvent.type reaches its offscreen textarea and produces no edit at all. Unit tests drive the editor through its API; real keyboard behaviour stays in the Storybook play functions, which run in Chromium. Two former keystroke-by-keystroke unit tests were rewritten to assert the same mechanism (each settled draft displayed verbatim) step by step.

Testing

• New: JsonEditor, EditReplayButton, EditReplayModal (tests + stories), SchemaForm raw JSON (#2151), ProtocolEntry > edit and replay (#2151), isJsonDocument, and two useExportActions override cases.
• npm run ci passes locally: 6,934 tests, per-file coverage gate clean, build gate, bundle-externals, smokes, Storybook.
• clients/web/README.md's "Code editing" section is rewritten as "JSON editing and display (JsonEditor)", with the contract table and the five load-bearing integration details.

Screenshots

Driven headlessly against a composable test server whose echo tool declares an
array argument and a property-less object argument — both of which fall through
to the JSON editor.

A tool's JSON argument

Before, the array default sits in a bare JsonInput; after, it has line
numbers, highlighting and fold widgets, and the form carries the Edit as
JSON switch. (The Options field below it is the property-less object,
opening empty.)

Before	After

Edit as JSON

The switch replaces the widgets with one editor over the whole arguments
object, seeded from what the form holds.

A tool result

A Protocol entry's payloads

ContentViewer's JSON branch, read-only. Note the before column renders the
payload as unhighlighted preformatted text — the copy button is in the same
place in both.

Before	After

Edit and replay

New. Seeded from the entry, Send disabled while the draft does not parse.

[Copilot]

Pull request overview

Introduces a shared Ace-based JSON editor across the web client, restores whole-argument JSON editing, and adds editable protocol replay.

Changes:

• Centralizes editable and read-only JSON rendering in JsonEditor.
• Adds SchemaForm raw-JSON mode and Protocol “Edit and replay.”
• Updates affected tests, stories, styling, and documentation.

Reviewed changes

Copilot reviewed 46 out of 46 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
clients/web/README.md	Documents JSON editor architecture.
clients/web/src/App.css	Hides read-only Ace cursors.
clients/web/src/test/aceEditor.ts	Adds labelled Ace test helpers.
clients/web/src/lib/protocolReplay.ts	Defines replay override states.
clients/web/src/hooks/useExportActions.ts	Dispatches edited replay parameters.
clients/web/src/hooks/useExportActions.test.tsx	Tests replay overrides.
clients/web/src/components/views/InspectorView/InspectorView.tsx	Propagates replay override typing.
clients/web/src/components/screens/ProtocolScreen/ProtocolScreen.tsx	Propagates replay override typing.
clients/web/src/components/elements/JsonEditor/JsonEditor.tsx	Implements shared Ace editor.
clients/web/src/components/elements/JsonEditor/JsonEditor.test.tsx	Tests editor behavior and accessibility.
clients/web/src/components/elements/JsonEditor/JsonEditor.stories.tsx	Adds editor stories.
clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx	Refactors object input onto JsonEditor.
clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx	Updates object-editor tests.
clients/web/src/components/elements/EditReplayButton/EditReplayButton.tsx	Adds replay-edit action.
clients/web/src/components/elements/EditReplayButton/EditReplayButton.test.tsx	Tests replay-edit action.
clients/web/src/components/elements/EditReplayButton/EditReplayButton.stories.tsx	Adds replay-edit stories.
clients/web/src/components/elements/ContentViewer/ContentViewer.tsx	Uses Ace for JSON display.
clients/web/src/components/elements/ContentViewer/ContentViewer.test.tsx	Tests JSON rendering paths.
clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts	Adds strict JSON-document detection.
clients/web/src/components/elements/ContentViewer/contentViewerUtils.test.ts	Tests JSON detection.
clients/web/src/components/elements/CodeHighlight/CodeHighlight.tsx	Removes Prism JSON loading.
clients/web/src/components/elements/CodeHighlight/CodeHighlight.test.tsx	Retargets highlighter tests to YAML.
clients/web/src/components/elements/CodeHighlight/CodeHighlight.stories.tsx	Replaces JSON story with YAML.
clients/web/src/components/groups/SchemaForm/SchemaForm.tsx	Adds Ace fields and raw mode.
clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx	Covers raw-JSON behavior.
clients/web/src/components/groups/SchemaForm/SchemaForm.stories.tsx	Demonstrates raw-JSON switching.
clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx	Implements editable replay modal.
clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx	Tests replay editing and validation.
clients/web/src/components/groups/EditReplayModal/EditReplayModal.stories.tsx	Adds replay-modal stories.
clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx	Integrates edit-and-replay controls.
clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.test.tsx	Tests protocol replay editing.
clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.tsx	Forwards replay overrides.
clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.test.tsx	Updates replay callback assertions.
clients/web/src/components/groups/MrtrConversation/MrtrConversation.tsx	Forwards replay overrides for rounds.
clients/web/src/components/groups/ImportServerJsonPanel/ImportServerJsonPanel.tsx	Replaces file textarea with Ace.
clients/web/src/components/groups/ImportServerJsonPanel/ImportServerJsonPanel.test.tsx	Updates import editor tests.
clients/web/src/components/groups/ServerImportJsonModal/ServerImportJsonModal.test.tsx	Drives imports through Ace helpers.
clients/web/src/components/groups/ExperimentalFeaturesPanel/ExperimentalFeaturesPanel.tsx	Replaces request textarea with Ace.
clients/web/src/components/groups/ExperimentalFeaturesPanel/ExperimentalFeaturesPanel.test.tsx	Updates request editor tests.
clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx	Updates JSON validity tests.
clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx	Updates tool JSON tests.
clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.test.tsx	Reads virtualized JSON through Ace.
clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.test.tsx	Verifies Ace-rendered output.
clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.test.tsx	Verifies inferred JSON rendering.
clients/web/src/components/groups/ResourceLink/ResourceLink.test.tsx	Updates inline resource assertions.
clients/web/src/components/groups/ResourceLink/ResourceLink.stories.tsx	Updates large-result interaction checks.
clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx	Updates masked-body assertions.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Review round 1 — all three comments addressed (c9fb461)

Mirrored here because inline replies get folded away once the fix is pushed and the threads go outdated.

1. maxLines={Infinity} defeats Ace's virtualization — fixed. Capped at JSON_DISPLAY_MAX_LINES = 200.

The reasoning I had written into that comment was wrong on the part that matters. I chose Infinity to avoid nesting a second scrollbar inside the host's scroll container, treating the DOM cost as no worse than the preformatted/Prism block it replaced — which holds for a payload the user is looking at, but not for the Protocol and Network lists, which are not virtualized and keep every entry's payload mounted inside its Collapse. Uncapped, virtualization can never engage there. 200 keeps both properties where each matters: below it the editor grows to fit so the host's scroller is the only one (essentially every real payload); above it Ace renders only the rows its viewport covers. Copying is unaffected — the overlay copies the original text, not what is rendered.

StructuredOutputPanel > Large now pins both halves, since "capped" and "truncated" look identical from outside: the last row is in the editor's document and is not in the DOM.

Not done, on the record rather than silently dismissed: capping bounds the per-payload DOM but not the per-payload instance count — a long Protocol history still constructs an Ace editor per collapsed payload. Deferring that mount means changing when ProtocolEntry/NetworkEntry render inside their Collapse, which their tests deliberately pin as "stays mounted", so it belongs in its own change. I'll file it as a follow-up.

2. disabled is invisible to keyboard and assistive tech — fixed. The effect that already wires the id / aria-describedby / aria-invalid onto Ace's hidden textarea now also sets aria-disabled and tabIndex = -1, and restores both when re-enabled. Both directions are tested, because this is derived state: a tool panel disables the whole form while a call is in flight and re-enables it on return, so the editor flips in place.

readOnly deliberately keeps tabindex="0" — a payload has to stay keyboard-scrollable (WCAG SC 2.1.1), which is the same reason the preformatted block it replaces carries tabIndex={0}. Only disabled leaves the tab order.

3. "sitting" typo — fixed.

One thing I added that Copilot did not ask for, because CI caught it: the new Disabled story scopes the axe color-contrast rule off. WCAG 1.4.3 exempts inactive components, and axe cannot see that the dimmed gutter belongs to one — the disabled state lives on Ace's hidden textarea while the text it measures is in the separately-rendered gutter. Scoped to that one story, so the rule still covers the enabled editor and the whole read-only path.

npm run ci passes: 6,937 tests, coverage gate, build gate, bundle-externals, smokes, 513 Storybook play functions.

[Copilot]

Pull request overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx:52

• JSON.parse accepts an overflowing number such as 1e400 as Infinity; isJsonObject then accepts the containing object, so Send remains enabled even though the edited params are not JSON-serializable and will be rejected or serialized as null. Apply the existing isSerializableJson guard before returning success, matching jsonObjectDraft.ts.

  let parsed: unknown;
  try {
    parsed = JSON.parse(text);
  } catch {
    return { ok: false, error: "Not valid JSON — this cannot be sent" };
  }
  if (!isJsonObject(parsed)) {
    return { ok: false, error: "Params must be a JSON object (`{ … }`)" };

clients/web/src/components/groups/SchemaForm/SchemaForm.tsx:582

• JSON.parse accepts overflowing literals such as 1e400 as Infinity, and this object-only check then treats the draft as valid. The form enables submission and emits a value that cannot be represented as JSON (JSON.stringify changes it to null). Validate with the existing isSerializableJson helper, as jsonObjectDraft.ts does, and keep the raw draft invalid when a number is non-finite.

[cliffhall]

Review round 2 — both comments addressed (b4083ee)

Including the two in the Suppressed comments block, which are the more valuable half of this round.

1. Raw JSON edits didn't move the Variant picker (inline) — fixed.

Nothing in values names a branch, so the picker's index is held by the form and was only re-derived when something else signalled the values had changed underneath (resetKey, a rewritten union). A raw edit is exactly that kind of change and had no such signal, so {"kind":"email"} → {"kind":"sms",…} left the picker on Email — and switching back to the widgets renders activeBranch's properties, so the form showed the Email fields over SMS values.

handleRawArgumentsChange now re-selects on every raw edit, so the picker tracks the document live rather than only on the way out. The ?? branchIndex fallback is load-bearing: the discriminator is absent for as long as it takes to clear and retype, and snapping to the first branch there would move the picker under the user mid-edit.

Fixing it surfaced a second problem I'd otherwise have shipped: the raw editor was keyed by draftKey (entity and branch), copying the per-field editors — so re-deriving the branch changed its key and remounted it, reseeding from values and reformatting the text being typed. It's now keyed by resetKey alone, which is the right level: it holds the whole arguments object rather than a per-branch slice, so a branch move should re-sync it (useValueChange already does) rather than replace it.

2 & 3. JSON.parse accepts 1e400 as Infinity (suppressed — both sites) — fixed, and in a third place you didn't flag.

This is the sharpest finding of the review, because it is the failure mode this codebase names explicitly: the editor shows 1e400, JSON.stringify writes null, and the Inspector reports something other than what it sent. parseJsonObjectDraft already carried isSerializableJson with that exact rationale in a comment; my two new parsers reimplemented the shape check and dropped the guard.

Both now have it. I also applied it to SchemaJsonField's parseJsonDraft, which you didn't flag because the function is older than this PR — but it is the same defect in the same family, in a file this PR already rewrites, and leaving it as the one unguarded parser of three is worse than fixing it. There it reports undefined, which the field already renders as an error and reports through onValidityChange, so submission is blocked rather than silently altered.

Three new tests cover the picker (moves / holds / doesn't reformat) and two cover the overflow rejection.

npm run ci passes: coverage gate, build gate, bundle-externals, smokes, 513 Storybook play functions.

---

Still outstanding from round 1, deliberately not in this PR: capping maxLines bounds the per-payload DOM but not the per-payload instance count, so a long Protocol history still constructs an Ace editor per collapsed payload. That needs ProtocolEntry/NetworkEntry to defer rendering inside their Collapse, which their tests pin as "stays mounted" behaviour — a separate change, and a virtualized Protocol list would subsume it.

[Copilot]

Pull request overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

clients/web/src/components/elements/JsonEditor/JsonEditor.tsx:200

• ariaLabel is only forced onto Ace’s hidden textarea in onLoad. If this prop changes while the editor remains mounted (for example, an in-place schema refresh changes a field title), setOptions updates Ace’s option but the DOM label is not recomputed until the cursor moves, so the visible label and accessible name disagree. Reapply the option and call textInput.setAriaLabel() from the wiring effect whenever ariaLabel changes.

  }, [wrapperId, hasError, description, disabled]);

clients/web/README.md:288

• This universal claim is inaccurate: ExperimentalFeaturesPanel still displays its JSON-RPC response through ResponseTextarea (ExperimentalFeaturesPanel.tsx:333), and decoded JWT JSON is rendered by OAuthTokenField.tsx:125. Narrow this to the JSON editing surfaces and the ContentViewer JSON branch unless those remaining displays are also migrated.

Every surface in this client where JSON is typed or displayed renders one element: **`elements/JsonEditor`**, an **Ace** editor (`react-ace` + `ace-builds`, declared in this client because they render React). Ace brings code folding, line numbers, brace auto-closing, and per-line error annotation from its JSON worker — the last three are why hand-writing a nested payload in a bare textarea was the actual pain ([#2151](https://github.com/modelcontextprotocol/inspector/issues/2151)).

clients/web/README.md:301

• ContentViewer is not the route for every read-only JSON payload in the client: the experimental JSON-RPC response and decoded JWT display bypass it. This architectural documentation should describe only ContentViewer consumers so it does not send future maintainers to the wrong abstraction.

**Read-only mode is what `ContentViewer` renders JSON as**, and through it every JSON payload in the app: Protocol and Network entries, tool results, structured output, resource previews, server cards. Highlighting is *not* what that buys — JSON already highlighted, via the lazily-imported Prism grammar `CodeHighlight` loads. What Ace adds is **folding**, line numbers and a gutter on a large payload. The Prism `json` grammar was dropped in the same change rather than kept beside it: two highlighters for one language drift, and nothing else asks for `json`. Two cases stay on the plain renderer — a `wrap={false}` caller (the server card's fixed-height, single-line box) and untyped text that only *looks* like JSON but does not parse, which in an editor would frame a server's prose as a malformed document.

[cliffhall]

Review round 3 — all three suppressed comments addressed (12c00d8)

No new inline comments this pass; the whole round was in the Suppressed comments block, and all three were right.

1. ariaLabel was only applied at mount — fixed.

setOptions does update Ace's option when the prop changes, but Ace recomputes the textarea's aria-label from it inside TextInput.setAriaLabel(), which it calls when the cursor moves. So a name that changed while the editor stayed mounted left the visible label and the accessible name disagreeing until the user happened to click into the box. That is reachable here rather than theoretical: SchemaJsonField passes the field title straight through as ariaLabel, and a tool refreshed in place can change it under a mounted form.

The wiring effect now re-applies it, with ariaLabel in its deps. onLoad keeps doing it as well — an effect runs after paint, and this is the control's only name, so mount should not present a frame without it. Test pins the rename.

2 & 3. The README overclaimed its own scope — fixed.

Checked both, and both are real:

• ExperimentalFeaturesPanel.tsx still renders its JSON-RPC response through ResponseTextarea. Only its request box moved — the issue enumerates the request box specifically, so this was scope, not oversight, but the README asserted otherwise.
• ConnectionInfoContent/OAuthTokenField.tsx renders a decoded JWT in a Code block beside the raw token, where the decoded/raw toggle and copy affordance belong to the field rather than to a viewer.

Neither goes through ContentViewer, so neither is reached by its JSON branch. The opening line is narrowed to "where JSON is typed, plus ContentViewer's read-only JSON branch", and the read-only section now names both exceptions explicitly rather than leaving a maintainer to discover them — which is your point 3's argument, and the right one: the value of that section is telling someone which abstraction to reach for.

I did not migrate them. Both are outside #2151's enumerated surfaces, and the JWT one in particular is a field with its own toggle rather than a payload viewer, so it deserves its own decision rather than being swept in at the end of a review round.

npm run ci passes: coverage gate, build gate, bundle-externals, smokes, 513 Storybook play functions. GitHub's checks were green on the previous head.

[Copilot]

Pull request overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

clients/web/src/components/elements/ContentViewer/ContentViewer.tsx:194

• This labels every focusable read-only Ace instance as “JSON content”. An expanded Protocol entry can contain both Parameters and Response editors (and the list can contain many entries), so screen-reader users tab through indistinguishable textboxes even though the visible sections have distinct names. Please let callers provide a contextual accessible name (for example, “Request parameters JSON” and “Response JSON”) and associate it with this editor.

      <JsonEditor
        ariaLabel="JSON content"
        value={formatJson(text)}
        readOnly

clients/web/src/components/elements/ContentViewer/ContentViewer.tsx:234

• isJsonDocument parses the payload here, then JsonContent immediately parses it again through formatJson. Protocol and Network entries pass their serialized bodies as untyped text, so a large, non-virtualized history now performs two full parses per JSON payload on each render. Return the formatted text from the validation step (or otherwise memoize a single parse/format result) before mounting Ace.

  if (wrap && isJsonDocument(text)) {
    return <JsonContent text={text} copyable={copyable} />;

[cliffhall]

Review round 4 — all three addressed (5abe289)

One inline, two suppressed. All three were right, and the inline one is the best catch of the review.

1. Edit-and-replay showed params that Send would drop (inline) — fixed.

replayProtocolRequest doesn't re-send a captured frame; it dispatches through the typed InspectorClient methods, whose signatures have nowhere to put _meta (a captured tools/call carries _meta.progressToken — visible in this PR's own screenshot) or any list-request key but cursor. For one-click Replay that lossiness is invisible and harmless: it re-sends whatever it was handed. Putting the params in front of the user to edit turns it into a misreport — the editor invites a change that Send silently discards.

Of the two options offered I took the second (restrict the editor), because threading _meta and the task options through means widening the InspectorClient call surface — a change to the client rather than to this feature, and one to make deliberately rather than at the end of a review round.

New replayableParams(method, params) lives beside the dispatch switch, since the two are halves of one contract. The modal seeds from its params and names its dropped:

Replay re-issues this request through the client's own tools/call method, which carries only the fields below, so _meta is not re-sent.

A non-string cursor counts as dropped rather than kept: the dispatcher's typeof … === "string" guard ignores it, so keeping it would put a value in the editor that changes nothing — the same defect one level down.

2. Every read-only JSON editor was named "JSON content" (suppressed) — fixed.

ContentViewer gains jsonLabel. An expanded Protocol entry holds two of these and the list holds many pairs, so a screen reader was tabbing through indistinguishable textboxes with only the visible headings to tell them apart. Protocol entries now name theirs per method and direction (tools/call request parameters JSON / tools/call response JSON), Network entries per request/response body. The default is unchanged for the single-payload consumers.

3. The payload was parsed twice per render (suppressed) — fixed.

isJsonDocument is now formatJsonDocument, returning the formatted text instead of a boolean, so the heuristic path no longer parses once to decide and again to render. That matters exactly where you said it does: Protocol and Network hand over serialized bodies as untyped text and are not virtualized.

npm run ci passes: coverage gate, build gate, bundle-externals, smokes, 513 Storybook play functions.

[Copilot]

Pull request overview

Copilot reviewed 49 out of 49 changed files in this pull request and generated 1 comment.

[cliffhall]

Review round 5 — the one comment addressed (4e7537e)

One inline comment, no suppressed ones.

Edits weren't projected, only the seed was — fixed.

Last round I stopped the editor offering a key the dispatcher would drop; this is the same defect one step later, and the example given is exact: a tools/list request has nothing dropped to warn about, so adding _meta to its draft hit no guard at all and Send discarded it silently.

parseParamsDraft now takes the method and asks the same function the seed is built from:

const { dropped } = replayableParams(method, parsed);
if (dropped.length > 0) {
  return { ok: false, error: `${formatDroppedKeys(dropped)} not carried by ${method} — remove …` };
}

Deriving both from replayableParams is the part that matters: the editor and the dispatcher now agree about which keys survive by construction, so a method added to the switch later cannot leave the validation behind.

I chose to disable Send over stripping on submit — consistent with the modal's other invalid states (unparseable, non-object, non-finite number), and because stripping would restore exactly the property this exists to remove: the editor showing something other than what is sent. It costs a user pasting a whole captured frame one deletion, which the note above the editor already predicts.

Three tests: the key is rejected, removing it re-enables Send, and — since the check is per method rather than a fixed key list — name/arguments pass on tools/call and are rejected on tools/list.

npm run ci passes: coverage gate, build gate, bundle-externals, smokes, 513 Storybook play functions. GitHub's checks were green on the previous head.

[Copilot]

Pull request overview

Copilot reviewed 49 out of 49 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx:428

• A declared JSON body does not reach the MIME-based JSON branch because contentType is not forwarded. As a result, malformed application/json bodies still use the plain-text renderer, even though this change explicitly treats declared JSON as JSON regardless of parse success. Pass the available MIME type to ContentViewer.

      <ContentViewer
        block={{ type: "text", text: body }}
        copyable
        jsonLabel={`${label} JSON`}
      />

clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx:88

• This accepts a present non-object arguments, so valid JSON such as {"name":"x","arguments":null} enables Send. replayProtocolRequest then applies params.arguments ?? {} and sends {}, not the value shown in the editor; arrays/scalars can likewise reach the typed call through a cast. Reject a present non-object arguments for tools/call and prompts/get before enabling Send.

  if (!isSerializableJson(parsed)) {

[cliffhall]

Review round 9 — all three addressed (03bf9ec)

Two of the three were consequences of round 8's own changes.

1. The tool was resolved from the entry's name, not the draft's — fixed.

I threaded the tool but looked it up in the wrong place: the modal lets name be edited, so retargeting an echo replay at add validated against echo's schema while the dispatch looked up add. The modal now takes the whole tools list and resolves from the draft's current name on every validation — the same lookup replayProtocolRequest does. ProtocolEntry no longer picks a tool at all, just forwards the list: less code, one fewer place to get this wrong. A test walks the retarget in both directions.

2. The error message contradicted its own check — fixed.

Written for the value-based version and not revisited when the rewrite changed what the rule is. Renamed IMPRECISE_INTEGER_ERROR, doc pointing at the helper that exists, and the text now describes exact representability rather than the safe-integer range — which is true of exactly the inputs it rejects, 2^54 being the case the old wording denied.

3. "Not valid JSON" misdiagnosed a valid document — fixed.

The worst direction for a message to be wrong in: the user reads about syntax while looking at syntactically fine JSON. describeJsonDraftProblem carries the parser's reason through, so the field distinguishes unparseable text, a non-finite number, and one that cannot be represented exactly. The shared "this field will be omitted" suffix stays, because the consequence really is the same.

---

Worth reporting, because it is the kind of thing this PR keeps turning up: the first version of fix 3 had a bug every assertion missed. Routing the value through the reason check meant empty text — not a problem, since an untouched optional field is fine — fell through to JSON.parse("") and threw out of the change handler. All 5,443 unit assertions still passed; it surfaced only as five unhandled errors across four suites, and only because npm run ci fails the run on those rather than reporting them as warnings. Excluded explicitly now, with a regression test.

npm run ci passes. One flake seen and confirmed unrelated: AppRenderer > pushes a live theme flip failed once under full-suite load and passes in isolation; this PR touches no AppRenderer file, and a clean re-run of the coverage gate is green.

[Copilot]

Pull request overview

Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts:165

• This formatter can change the value being inspected. For example, {"id":9007199254740993} is rendered with 9007199254740992, and {"limit":1e400} is rendered as null, even though the copy action still contains the original text. Preserve the source for overflowing/imprecise numeric literals (the new hasImpreciseIntegerLiteral and existing serializability check can gate pretty-printing), or use a lossless JSON formatter; apply the same rule to the declared-JSON formatJson path so both read-only branches remain faithful to the wire payload.

    return JSON.stringify(JSON.parse(text), null, 2);

clients/web/src/utils/jsonObjectDraft.ts:48

• Skipping every fractional or exponent-form token still permits the editor/wire mismatch this guard is meant to prevent. For example, {"n":0.10000000000000001} parses and serializes as {"n":0.1}, so the editor keeps digits that submission drops; long exponent-form mantissas have the same problem. Compare the exact decimal value of every numeric token with the parsed number's serialized value, regardless of notation, and only accept it when they are equivalent.

[cliffhall]

Review round 10 — first suppressed comment fixed, second declined with reasons (390e0c4)

No new inline comments. Two suppressed; I've taken one and am pushing back on the other.

Fixed: the read-only viewer could reformat a payload into a different one

This is a good catch and the sharpest kind for this project. formatJson pretty-prints by JSON.parse then JSON.stringify — a round trip through JavaScript's number type, which is not lossless. A server sending {"id":9007199254740993} was shown …992; one sending {"limit":1e400} was shown null. Neither is a formatting difference. Both are the viewer telling the reader something untrue about the payload it received, which for an inspector is worse than any of the editor-side cases this PR has been fixing — there the user is about to send something, here they are trying to find out what happened.

It predates this PR (formatJson has always round-tripped), but this PR is where the read-only JSON path was rebuilt, and the two predicates the draft parsers already use answer it exactly. Both formatters now display the payload as it arrived when reindenting would alter a value — unindented, but honest. The copy button already copied the original text, so this makes the screen agree with it.

Declined: rejecting literals whose spelling changes but whose value does not

{"n":0.10000000000000001} and {"n":0.1} are the same double — JSON.parse("0.10000000000000001") === 0.1 is true. So the wire carries exactly the number the literal denotes; what changes is only its spelling, and normalizing spelling is what serialization does to every value (1e2 → 100, key order, whitespace).

The comparison suggested — the token against the parsed number's serialized form — is a test for "already in JS's shortest form", and it rejects 1.50, 1e2, 0.5000, 1.0. Blocking Send on a draft that sends exactly what it means is a worse failure than the one it prevents, and it would make the editor unusable for anyone who writes 1.50 because the neighbouring line does.

There is no middle option, either: any "exact decimal value vs the double" test rejects 0.1 itself, since 0.1 is not exactly representable.

The integer guard stays because it is a different thing. 9007199254740993 names an integer the double is not — for an id, the digits are the identity, and sending …992 targets a different record. That is a changed value, not a changed spelling, and it is the line the guard draws.

If you'd rather surface the normalization without blocking, the honest shape would be a non-blocking note ("this will be sent as 0.1") rather than a validation failure — happy to add that if the maintainer wants it, but it is a product decision rather than a correctness one.

npm run ci passes: coverage gate, build gate, bundle-externals, smokes, 513 Storybook play functions.

[Copilot]

Pull request overview

Copilot reviewed 52 out of 52 changed files in this pull request and generated 3 comments.

[cliffhall]

Review round 11 — all three addressed (1b28fc8)

Three comments, one defect in three places, and a good one.

JSON.parse accepts duplicate member names and keeps the last without complaint, so {"role":"user","role":"admin"} becomes {"role":"admin"} — a document that renders, and submits, as less than it says. In the read-only viewer that is the Inspector hiding what the server sent; in a draft it is the editor showing more than the wire will carry. Nothing downstream could have caught it, because by the time JSON.parse returns the evidence is gone — which is why it needed a source scan rather than another check on the parsed value.

New findDuplicateObjectKey in utils/jsonObjectDraft:

• Scoped per object, so {"a":{"a":1}} and [{"a":1},{"a":2}] are fine — the repeat has to be within one set of braces.
• Compares names by decoded value, so {"a":1,"a":2} is caught; comparing raw source would miss a duplicate written the second way.
• Distinguishes names from values. A string value that repeats is not a duplicate, and {"a":"}\"a\":1{","b":2} — a value containing what looks like a key — is not either.
• Says nothing about a half-typed draft, leaving the parse to report the real problem.

It shares its escape handling with the number-literal scanner, which now reads strings through the same helper instead of skipping them inline — one place to get backslash-before-quote right.

Wired into all four draft parsers and into the viewer's faithfulness check, so the same document is refused, or shown unindented, everywhere. Twelve new tests.

npm run ci passes: coverage gate, build gate, bundle-externals, smokes, 513 Storybook play functions.

---

A note on where this is heading, since it is round eleven. Every finding since round 7 has been an instance of one principle — what the editor shows must be what the wire carries, and what the viewer shows must be what the wire carried — and each round has found a subtler instance: rounded integers, then exponent-form literals, then string coercion per schema, then the retargeted tool, and now duplicate keys. That has been genuinely valuable and I have taken all of it. But the remaining surface is JSON-parsing exotica rather than anything this feature introduced, and the guards now live in one shared place with tests. If the next round surfaces something in that same family, I'd suggest it lands as a follow-up issue against utils/jsonObjectDraft rather than continuing to grow this PR — happy to file it either way.

[Copilot]

Pull request overview

Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/utils/jsonObjectDraft.ts:51

• Negative-zero literals bypass this comparison: BigInt("-0") and BigInt(Number("-0")) are both 0n, but JSON.stringify(JSON.parse('{"n":-0}')) emits {"n":0}. As a result, the raw form and replay editor can show -0 while sending 0, and ContentViewer can rewrite a received -0 during pretty-printing. Detect Object.is(Number(literal), -0) for all numeric spellings (including -0.0/-0e1) before the integer-only check, and use a diagnostic that covers serialization loss as well as rounding.

[cliffhall]

Review round 12 — one fixed here (8aa6f01), one filed as #2171

Negative-zero literals (suppressed) — fixed.

-0 is represented exactly, so the digit comparison had nothing to say about it — but JSON.stringify writes it as 0, so the loss happens on the way out rather than on the way in. The editor showed -0 and the wire carried 0; the viewer rewrote a received -0 while pretty-printing.

hasImpreciseIntegerLiteral is now findUnsendableNumberLiteral, checking Object.is(Number(literal), -0) for every spelling — -0, -0.0, -0e1, -0.0e-5 — before the integer-only digit check. It returns the offending literal rather than a boolean, so the message can name it, which is more use than restating either of the two rules behind it and matches how the duplicate-key message reads.

Raw-mode arguments are still coerced by the schema (inline) — filed as #2171, not fixed here.

This one is real, and the second-order effect is the sharper half: {"count":"01"} is sent as 1, so a schema-invalid argument cannot be sent at all — which is part of why a raw-JSON escape hatch is wanted.

I've deliberately not fixed it in this PR, and the issue records why. The change spans core/ (callTool, callToolStream and the task path each need a coercion-skip option, on shared code the CLI and TUI also call) and a long thread through SchemaForm → the detail panels → App.tsx's three call sites. More importantly it rests on a product question a review round should not settle: should the Tools tab be able to send arguments the schema calls invalid? Raw mode's purpose argues yes; if the answer is no, the right fix is the opposite one — refuse the coercion, as this PR's Edit-and-replay modal already does for the identical conversion — and the two paths should agree. #2171 sets out both options and asks for the decision; it's on the board in Todo against v2.5.0.

npm run ci passes: coverage gate, build gate, bundle-externals, smokes, 513 Storybook play functions.

---

This is where I'd suggest the review lands. Twelve rounds have produced 28 findings, all taken except two I've argued against on the record (normalized number spellings, which change no value) and this one, which is now tracked. Everything the PR itself introduced is fixed; what is left is a shared-core decision with an issue behind it.

[Copilot]

Pull request overview

Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/components/elements/ContentViewer/ContentViewer.tsx:478

• The new JSON renderer is propagated only through the text block branch. Embedded-resource ContentBlocks take the separate resource case below, which ignores block.resource.mimeType; therefore a prompt message containing { type: "resource", resource: { mimeType: "application/json", text: "{\"a\":1}" } } still renders as plain Code. Route embedded text resources through the same MIME/JSON dispatch so all ContentViewer consumers actually inherit this migration.

          mimeType={mimeType}
          copyable={copyable}
          wrap={wrap}
          jsonLabel={jsonLabel}

[cliffhall]

Review round 13 — both addressed (684eb9c)

1. 1e-400 underflows to zero (inline) — fixed.

A nonzero mantissa that parses to 0. The digit comparison skips it for being exponent-form and it is not negative zero, so nothing here saw a document that says one number and sends none of it.

if (Number(literal) === 0 && /[1-9]/.test(mantissaOf(literal))) return literal;

Pairing the two conditions is what keeps it from over-reaching: a literal written as zero in exponent form (0e-400) stays acceptable, and a subnormal that still parses to something (1e-320) is a real value rather than an underflow to nothing.

2. Embedded resource blocks bypassed the dispatch (suppressed) — fixed.

Right, and it undercut a claim this PR makes: the resource branch rendered every embedded resource in a plain code block, ignoring resource.mimeType, so a prompt message carrying JSON did not inherit the migration even though the text-block branch did. An embedded text resource declaring JSON now goes through the same dispatch.

Narrowed to JSON, on the same reasoning as the Network tab's bodies: routing every declared type through would also put an embedded text/html resource into the sandboxed frame and a text/csv one into a table. The Resources screen already renders resource contents that way, so doing it here too is defensible — but it changes what a tool result is, and belongs to whoever decides that rather than to a JSON-renderer swap. Both halves are pinned by tests.

npm run ci passes: coverage gate, build gate, bundle-externals, smokes, 513 Storybook play functions.

[Copilot]

Pull request overview

Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

clients/web/src/lib/protocolReplay.ts:188

• An omitted arguments key is also reshaped: both replay branches pass params?.arguments ?? {}, and the client emits that empty object on the wire. Thus the editor can show {"name":"echo"} while Send transmits {"name":"echo","arguments":{}}, despite this helper's contract that accepted params survive intact. Seed the editor with arguments: {} when replay will add it, or preserve omission in the dispatcher.
  clients/web/src/components/groups/ImportServerJsonPanel/ImportServerJsonPanel.tsx:152
• Using hasContent here removes the Clear action for a whitespace-only draft because that flag trims the text. The editor still contains the whitespace (and the previous textarea showed Clear for any non-empty string), so this is a small regression. Keep the trimmed flag for validation/display logic, but show Clear whenever draft.rawText.length > 0.

          {hasContent && (

[Copilot]

Pull request overview

Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.

Suppressed comments (1)

clients/web/src/utils/jsonObjectDraft.ts:62

• This skips every fraction/exponent token, but those spellings can still encode an exact integer that JSON.parse rounds. For example, both 9007199254740993e0 and 9007199254740993.0 become 9007199254740992, so the raw editor/replay path sends a different value and ContentViewer pretty-prints a payload as the wrong value despite this guard. Please normalize mathematically integral decimal/exponent literals and compare their exact value with the parsed number too, while continuing to accept genuinely fractional/exponent values that round-trip as intended.

[cliffhall]

Review loop complete

Two further review requests (10:27Z and 11:27Z) were dequeued without a review; the last one posted was round 13's, and every round before it landed within 5–21 minutes. Recording it here so the silence reads as "nothing further" rather than "still pending".

Thirteen rounds, 30 findings. All were taken except three, each argued on the record rather than dropped:

Not taken	Why
Reject literals whose spelling normalizes (0.10000000000000001 → 0.1)	Same double — the wire carries what the literal denotes. The test for it rejects 1.50, 1e2, 0.5000, blocking drafts that send exactly what they mean.
Preserve empty cursors in four core/ list adapters	They look wrong (listTools deliberately carries ""), but they are shared with the CLI and TUI. replayableParams describes what the dispatch does instead.
Bypass schema coercion for raw-mode tool arguments	Real, and caused by this PR — filed as #2171. Spans core/ plus a long UI thread, and rests on a product question a review round should not settle.

The findings converged on one principle and kept finding subtler instances of it: what the editor shows must be what the wire carries, and what the viewer shows must be what the wire carried. Rounded integers, then full-form literals at ≥1e21, then per-schema string coercion, then the retargeted tool, then duplicate keys, then negative zero, then underflow. Two of those were bugs in my own earlier fixes, and one — the read-only viewer rewriting a received payload — was the most valuable catch of the review, because it predates this PR and affects reading rather than sending.

Everything this PR introduced is fixed. npm run ci and GitHub's checks are green on 684eb9c7.