Skip to content

Network: modern Mcp-* headers, new error codes, cancellation-as-abort (#1628)#1722

Merged
cliffhall merged 15 commits into
v2/mainfrom
v2/1628-network-modern-headers
Jul 18, 2026
Merged

Network: modern Mcp-* headers, new error codes, cancellation-as-abort (#1628)#1722
cliffhall merged 15 commits into
v2/mainfrom
v2/1628-network-modern-headers

Conversation

@cliffhall

@cliffhall cliffhall commented Jul 18, 2026

Copy link
Copy Markdown
Member

Closes #1628

Upgrades the Inspector's monitoring tabs for the modern (2026-07-28) Streamable HTTP transport — the standardized HTTP headers (SEP-2243) and the new JSON-RPC error taxonomy (SEP-2575). The Network tab owns the HTTP transaction; the modern protocol errors are surfaced distinctly in the Protocol tab, with a cross-tab link back to the correlated HTTP request.

Network tab (HTTP transaction)

  • Modern mirrored headersMcp-Method, Mcp-Name, Mcp-Param-*, MCP-Protocol-Version are recognised and highlighted (violet) in the request/response header tables. =?base64?{b64}?= sentinel values are decoded inline and flagged with a base64 chip (raw value on hover).
  • Header/body consistency — each mirrored standard header is cross-checked against the JSON-RPC request body (Mcp-Methodmethod, Mcp-Nameparams.name/uri, MCP-Protocol-Version_meta version). A disagreement is flagged in the danger colour with a labelled marker — a HeaderMismatch is visible before the server rejects it. This is a header-level concern, so it stays in the Network tab.
  • Cancellation-as-abort — a cancelled request renders as a neutral "Cancelled" (connection abort), not a hard error, with a note that the modern transport aborts the stream instead of sending a notifications/cancelled frame (SEP §7.5).
  • Browser-path mirroring — verified & documented. The SDK builds the standard mirrored headers unconditionally, so they reach the server through the web proxy (/api/fetch forwards them verbatim — integration-tested). It gates Mcp-Param-* on a non-browser environment (detectProbeEnvironment() !== "browser"), so the web client never emits them — an x-mcp-header tool is therefore uncallable from the web client against a strict modern server (it answers -32020); it works from the Node CLI/TUI. Documented in createRemoteFetch.ts.

The Network tab does not show the spec-error chip/alert — those live in the Protocol tab.

Protocol tab (protocol errors)

  • Distinct spec-error chip for -32020 HeaderMismatch, -32021 MissingRequiredClientCapability, -32022 UnsupportedProtocolVersion, -32601 MethodNotFound, colour-coded and shown in the entry's own casing (e.g. UnsupportedProtocolVersion, not run-together uppercase). Placement: wide layout → right after the method chip; compact sidebar row → no chip (the ERROR status badge suffices). An explanatory alert in the expanded detail names the code, describes it, and lists the server's supported versions for -32022.
  • Surfacing SDK-thrown errors. The SDK delivers -32020/-32021/-32022 (HTTP 400) as JSON-RPC error responses, so they land in the Protocol log natively. -32601 (HTTP 404) and other non-modern-enveloped errors are thrown — the request stays pending. enrichProtocolEntries (in App) correlates a pending Protocol request to its transport fetch by JSON-RPC id (parsed from FetchRequestEntry.requestBody, which is the serialized message) and folds the fetch's error onto it — so -32601 appears in the Protocol tab too. This is a derived view: the underlying MessageLogState is not mutated, and the memo returns the same array reference when nothing matched.

"Reveal in Network" link

The expanded Protocol alert can show a "View the HTTP request in the Network tab →" link. Clicking it correlates the message to its fetch, clears any filter that would hide the target, switches to the Network view in whichever mode is active, and scrolls to + force-expands the matching entry:

  • Sidebar pinned: switches the monitoring column to Network (pinMonitor).
  • Sidebar closed: the monitor screens are full-screen header tabs → onActiveTabChange("Network").
  • One-shot: a revealId threads NetworkScreen → NetworkStreamPanel → NetworkEntry, where the target scrolls itself into view in a requestAnimationFrame (landing after useScrollMemory's restore), expands, then clears the signal.

The link only appears when the Network entry adds value the alert lacks — i.e. the error is thrown or tied to the HTTP headers:

Error Arrives as Header-tied Link
-32020 HeaderMismatch delivered yes yes — the mirrored headers are the problem
-32601 MethodNotFound thrown yes — the real 404 response only exists in the Network log
-32021 MissingRequiredClientCapability delivered no no — protocol-only
-32022 UnsupportedProtocolVersion delivered no no — the supported list is already in the alert

Other changes

  • Redaction fix (core): the fetch-tracking body redaction masked any field named code (targeting the OAuth authorization code), which clobbered the numeric JSON-RPC error.code and silently broke the spec-error classification against real traffic. Redaction now only masks string values of a sensitive field.
  • Composable showcase server: test-servers/configs/modern-network-http.json serves a get_weather tool with an x-mcp-header annotation plus four trigger_* tools backed by a gated transport.modern.injectSpecErrors middleware that returns real 400/-32020, 400/-32021, 400/-32022 (+supported), 404/-32601 responses — so the whole taxonomy can be driven live. Documented in the root README.
  • The optional raw-request controls (cacheMode bypass/refresh, single-page fetch) are split to Network/Feature tabs: raw-request controls — cacheMode (bypass/refresh) + single-page fetch #1721 — cross-cutting core + feature-tab work beyond this card.
  • AGENTS.md board Status option-IDs refreshed (the Building … columns were removed).

Verification

  • npm run ci green on every commit (format, lint, build, per-file ≥90 coverage incl. the web integration project, smoke, Storybook).
  • Correlation/classification/enrichment have unit tests; ProtocolEntry and NetworkEntry cover the chip, alert, surfaced -32601, the link gating, and the reveal (force-expand + scrollIntoView + one-shot clear).
  • Driven live against the showcase server: each error renders distinctly in the Protocol tab (including the thrown -32601), and the reveal link jumps to + expands the correlated HTTP entry in both the pinned-sidebar and closed/full-screen layouts.

Screenshots

1. Protocol-only errors: An error that is only contained in a protocol response body has no link to the corresponding network entry in the error description.
protocol-only-error

2. HTTP-relevant protocol errors: Header-related protocol errors, or those thrown from server with a specific HTTP status code instead of just included in a protocol response have a link to the corresponding network entry in the error description.
reveal-1-protocol-link

3. Follow Link to Network entry (pinned sidebar): the corresponding HTTP entry scrolled to + expanded, with the highlighted Mcp-* headers
reveal-2-network-landed

4. Follow Link to Network entry (closed sidebar / full-screen): same jump, full-screen Network view
reveal-3-fullscreen-landed

5. HTTP-relevant protocol errors — thrown variant (-32601): A method-not-found is thrown by the server (HTTP 404) rather than returned in a protocol response; it's surfaced into the Protocol tab by correlation and, being HTTP-relevant, also carries the link.
methodnotfound-1-protocol-link

6. Follow Link to Network entry (thrown -32601): the correlated 404 NOT FOUND entry scrolled to + expanded — the real response that lives only in the Network log.
methodnotfound-2-network-404

7. Cancellation-as-abort: Shown as "Cancelled" status in protocol entry + the SEP-2575 note
cancellation-as-abort

8. Header/body mismatch marker (Network tab): A mirrored header that disagrees with the body is flagged in red with a ⊘ marker (rendered from the NetworkEntry component; a conformant client never produces this on the wire).
header-body-mismatch-marker

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

…#1628)

Upgrade the Network tab for the modern Streamable HTTP transport
(SEP-2243 / SEP-2575):

- Recognise and highlight the mirrored headers (Mcp-Method, Mcp-Name,
  Mcp-Param-*, MCP-Protocol-Version), decode =?base64?…?= sentinel values
  inline, and cross-check each against the request body so a HeaderMismatch
  is visible before the server rejects it.
- Show the new spec error codes distinctly: -32020 HeaderMismatch,
  -32021 MissingRequiredClientCapability, -32022 UnsupportedProtocolVersion
  (with the server-advertised `supported` versions), and modern -32601
  MethodNotFound on HTTP 404 (distinguished from a legacy bare 404 by its
  JSON-RPC body).
- Reflect cancellation as a connection abort ("Cancelled") rather than a
  hard error, per SEP §7.5.
- Confirm and document browser-path header mirroring: the SDK builds the
  Mcp-* headers client-side and the /api/fetch proxy forwards them verbatim
  (new integration test).

New pure util `mcpNetworkHeaders.ts` holds the recognition/decode/classify
logic; new `McpErrorBadge` element renders the distinct error chips. The
optional raw-request controls (cacheMode bypass/refresh, single-page fetch)
are split to #1721 — they are cross-cutting core + feature-tab work beyond
the Network tab.

Also refreshes the AGENTS.md board Status option-ID table (the Building /
MCP Apps Extension columns were removed).

Closes #1628

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 18, 2026
… codes

Adds a composable "modern-network" showcase server for exercising the #1628
Network-tab work end-to-end, and fixes a real bug the smoke test surfaced.

Bug fix (core): the fetch-tracking body redaction masked ANY field named
`code` (targeting the OAuth authorization code), including the numeric
JSON-RPC `error.code`. That destroyed the field the Network tab classifies
the modern spec errors on, so -32020/-32021/-32022/-32601 never rendered
against real traffic. Redaction now only masks STRING values of a sensitive
field (secrets are always strings; a numeric code is never one).

Test-server showcase (test-servers): `modern-network-http.json` serves a
`get_weather` tool with an `x-mcp-header: "City"` annotation plus four
`trigger_*` tools. A new `transport.modern.injectSpecErrors` flag installs a
middleware on the modern leg that answers the triggers with real HTTP status
+ JSON-RPC error bodies (400/-32020, 400/-32021, 400/-32022 w/ `supported`,
404/-32601), so the Inspector's error rendering can be driven live.

Mirroring finding (verified live, comment corrected in createRemoteFetch.ts):
the SDK builds the STANDARD mirrored headers (Mcp-Method / Mcp-Name /
MCP-Protocol-Version) unconditionally — they reach the server through the
web proxy — but gates `Mcp-Param-*` on a non-browser environment
(`detectProbeEnvironment() !== "browser"`), so the web client never emits
them. An x-mcp-header tool is therefore uncallable from the web client
against a strict modern server (it answers -32020); it works from the Node
CLI/TUI. The earlier "mirroring works through the proxy" note was corrected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Smoke-tested end-to-end + one bug fix + a mirroring correction

Added a composable modern-network-http.json showcase server (a get_weather tool with an x-mcp-header: "City" annotation, plus four trigger_* tools backed by a new transport.modern.injectSpecErrors middleware that returns real 400/-32020, 400/-32021, 400/-32022+supported, 404/-32601 responses) and drove the Network tab against it over a real modern (2026-07-28) connection.

Bug the smoke test caught (fixed here): the fetch-tracking body redaction masked any field named code (for the OAuth authorization code), which clobbered the numeric JSON-RPC error.code — so the spec-error classification silently failed against real traffic (it passed in unit tests/stories because those fixtures aren't redacted). Redaction now only masks string values of a sensitive field; a numeric code is never a secret. Added tests.

Mirroring finding — corrects the earlier claim. Verified live that the SDK builds the standard mirrored headers (Mcp-Method/Mcp-Name/MCP-Protocol-Version) unconditionally, so they reach the server through the web proxy. But it gates Mcp-Param-* on detectProbeEnvironment() !== "browser", so the web client never emits them — calling get_weather from the web produced a real -32020 from the SDK server (arguments in the body, no Mcp-Param-City header). So: standard headers mirror on the browser path; Mcp-Param-* does not (works from the Node CLI/TUI). The createRemoteFetch.ts comment is corrected accordingly.

Screenshots (live showcase + the three browser-path-impossible states via Storybook): all four error codes render as distinct badges; the -32022 alert lists the server's supported versions; a real -32020 HeaderMismatch; Mcp-Param-City decoded from its base64 sentinel with a BASE64 chip; the header/body mismatch marker; and cancellation-as-abort. npm run ci green.

cliffhall and others added 8 commits July 18, 2026 10:09
In the narrow monitoring sidebar (embedded layout) the McpErrorBadge shared
the top row with the timestamp/method/category/duration/status chips, which
truncated the method and category ("PO…", "TRANSP…") and clipped the error
name. Render the badge separately from `metaBadges` and place it on the
second row, to the right of the URL, so the top row stays readable and the
error name shows in full. The full-width layout keeps it in the top-right
cluster (where there's room).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
…case)

The four spec/SDK error identifiers are PascalCase (`HeaderMismatch`,
`MissingRequiredClientCapability`, `UnsupportedProtocolVersion`,
`MethodNotFound`). Mantine's Badge uppercases label text by default, which
flattened them to a hard-to-read run-together string
("UNSUPPORTEDPROTOCOLVERSION"). Set `tt="none"` so the badge shows the
spec's own casing with word boundaries intact; short badges (POST, status)
keep the default uppercase.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
…econd row

The full-width entry kept the McpErrorBadge in the top-right cluster, where a
long spec name (e.g. "MissingRequiredClientCapability") crowded the top row
and truncated the method chips and URL. Move it to the start of the second
row (the expand-toggle row) so the top row stays readable; the toggle stays
at the end (`space-between` only when the badge is present). The compact
sidebar already places it on its second row beside the URL.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
The compact (sidebar) ProtocolEntry already renders Replay as an icon next to
the pin/expand icons; the wide layout used a left-aligned "Replay" text
button instead. Make the wide layout match — Replay as the ReplayButton icon
at the start of the right-side action cluster (Replay, Pin, Expand) — so the
two layouts are consistent. Drops the now-unused SubtleButton constant.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
…rotocol tab

The Network tab is focused on the HTTP transaction; the distinct protocol-error
taxonomy (-32020/-32021/-32022/-32601) belongs with the protocol messages. So:

- ProtocolEntry now renders the McpErrorBadge chip on the top line (right after
  the direction badge, both layouts) plus an explanatory alert in the expanded
  detail, classified by JSON-RPC error code via `classifyProtocolSpecError`.
- NetworkEntry drops the chip + alert; it keeps the HTTP-level features (status
  badge, Mcp-* header highlight/sentinel-decode, header/body mismatch marker,
  cancellation-as-abort).
- The SDK delivers -32020/21/22 (HTTP 400) as JSON-RPC error responses, so they
  reach the Protocol log directly. -32601 (HTTP 404) and other non-enveloped
  errors are *thrown* — the request stays pending. `enrichProtocolEntries`
  (App) correlates the Protocol log to the Network log by JSON-RPC id (parsed
  from the transport fetch's requestBody) and folds the thrown error onto its
  pending request, so it too appears in Protocol. This is a derived view; the
  underlying log is not mutated.

Verified live against the modern-network showcase: both a -32020 error response
and a thrown -32601 render distinctly in the Protocol tab.

Follow-up: a "reveal in Network" link on the Protocol alert (cross-tab
scroll-to + expand of the correlated HTTP entry).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
… method

Refine the placement of the spec-error chip added in the previous commit:
- Wide layout: move it from after the direction badge to right after the
  method chip (reads as "tools/call  -32020 HeaderMismatch").
- Compact sidebar row: drop the chip entirely — the two-line row stays
  uncluttered and relies on its ERROR status badge. The explanatory alert
  still appears in the expanded detail in both layouts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
…TP entry

Adds the cross-tab navigation that was the follow-up to relocating the
spec-error rendering. The expanded Protocol alert now shows a "View the HTTP
request in the Network tab" link when the entry has a correlated Network
request; clicking it:

- correlates the Protocol message to its transport fetch by JSON-RPC id
  (`revealableMessageIds` gates the link; `correlateFetchEntry` finds the
  target on click),
- clears whatever filter would hide the target (network filterText +
  category, and the embedded monitorSearch),
- switches to the Network view in whichever mode is active — the pinned
  column (`pinMonitor`) when the sidebar is open, or the full-screen header
  tab (`onActiveTabChange`) when it's closed,
- scrolls the matching NetworkEntry into view and force-expands it.

The reveal is a one-shot: a new `revealId` threads NetworkScreen →
NetworkStreamPanel → NetworkEntry, where the targeted entry scrolls itself
into view (rAF, after `useScrollMemory`'s restore) + expands, then clears the
signal via `onRevealComplete`. Verified live in both the pinned-sidebar and
closed/full-screen layouts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
Gate the link on a new `httpRelevant` flag: show it only when the error is
either thrown by the SDK (its real HTTP response lives only in the Network
log — the Protocol entry is a synthetic fold) OR tied to the HTTP request
headers. A delivered, protocol-only error gets no link, since the raw HTTP
entry adds nothing over the alert.

- -32020 HeaderMismatch  → link (the mirrored headers are the problem)
- -32601 MethodNotFound  → link (thrown; the 404 response is only in Network)
- -32021 MissingRequiredClientCapability → no link (delivered, protocol-only)
- -32022 UnsupportedProtocolVersion      → no link (supported list is in the alert)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Design decisions: relocating the spec-error UX to the Protocol tab + "reveal in Network"

This documents the second half of the PR (commits after the initial Network-tab work), where the modern spec-error rendering was moved out of the Network tab and into the Protocol tab, with a cross-tab link back to the HTTP entry.

1. Why move the chip + alert to the Protocol tab

The Network tab is focused on the HTTP transaction (status, mirrored Mcp-* headers, header/body consistency, cancellation). The distinct protocol-error taxonomy (-32020/-32021/-32022/-32601) is a JSON-RPC / protocol concern and belongs with the protocol messages. Keeping a prominent error chip on every Network row was visual clutter that duplicated what the expanded entry already showed. So the chip + alert now live on the Protocol entry; the Network tab keeps only its HTTP-level features.

2. Surfacing SDK-thrown errors into the Protocol log (the correlation)

The two logs (MessageEntry / FetchRequestEntry) share no key, and the SDK does not deliver all spec errors as JSON-RPC frames:

Error HTTP SDK behavior In Protocol log?
-32020 / -32021 / -32022 400 delivered via onmessage (modern-enveloped request, matching id) yes — as the request's error response
-32601 404 thrown (SdkHttpError); request stays pending no — only in the Network log

To make -32601 (and any thrown/non-enveloped error) appear in the Protocol tab, enrichProtocolEntries (in App) correlates a pending Protocol request to its transport fetch by JSON-RPC id (parsed from FetchRequestEntry.requestBody, which is the serialized message) and folds the fetch's JSON-RPC error onto the request as a synthetic response.

Decision: this is a derived view, not a mutation of the core log. The MessageLogState stays truthful; the enrichment happens in an App-level useMemo and returns the same array reference when nothing matched (so the Protocol view only re-renders when an enrichment actually applies). A delivered error carries a duration; a thrown/enriched one does not — a useful tell when eyeballing.

3. Chip placement

  • Wide layout: chip sits right after the method chip (tools/call -32020 HeaderMismatch).
  • Compact sidebar row: no chip — the two-line row relies on its ERROR status badge to stay uncluttered.
  • The explanatory alert (name, description, supported list for -32022) shows in the expanded detail in both layouts.

4. "Reveal in Network" link + mode-aware navigation

The expanded alert can show a "View the HTTP request in the Network tab →" link. Clicking it correlates the message to its fetch, clears any filter that would hide the target (network text/category, or the embedded search), switches to the Network view in whichever mode is active, and scrolls to + force-expands the matching entry.

  • Sidebar pinned (open): switches the monitoring column to Network (pinMonitor).
  • Sidebar closed: the monitor screens are full-screen header tabs, so it uses onActiveTabChange("Network").
  • The reveal is a one-shot: a revealId threads NetworkScreen → NetworkStreamPanel → NetworkEntry, where the targeted entry scrolls itself into view in a requestAnimationFrame (so it lands after useScrollMemory's layout-effect restore, which would otherwise fight it), expands, then clears the signal via onRevealComplete. Verified live in both layouts.

5. When to show the link (the gating rule)

The link only appears when the Network entry adds diagnostic value the Protocol alert lacks — i.e. the error is thrown or tied to the HTTP request headers. A delivered, protocol-only error gets no link. Encoded as an httpRelevant flag per spec-error code:

Error Arrives as Header-tied Link
-32020 HeaderMismatch delivered yes yes — the mirrored headers are the problem
-32601 MethodNotFound thrown yes — the real 404 response only exists in the Network log
-32021 MissingRequiredClientCapability delivered no no — protocol-only, HTTP entry adds nothing
-32022 UnsupportedProtocolVersion delivered no no — the supported list is already in the alert

Testing

enrichProtocolEntries / correlateFetchEntry / revealableMessageIds have unit tests (id parsing, batch/non-JSON bodies, auth-category skip, string ids, data preservation, referential stability). ProtocolEntry has tests for the chip (wide-only), the alert, the surfaced -32601, the link (shown for thrown/header, hidden for -32021), and NetworkEntry has a test for the reveal (force-expand + scrollIntoView + one-shot clear). npm run ci green on every commit.

…c's §7.5

The cancellation note referenced "(SEP §7.5)", but §7.5 is a section of the
internal v2_new_spec_impact.md doc, not a SEP. The behavior — closing the
Streamable HTTP request stream is the cancellation signal; notifications/cancelled
is stdio-only — is defined in SEP-2575 (stateless-mcp). Cite SEP-2575 in the
user-facing alert and the two related comments.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 3m 18s —— View job


Review: Network modern Mcp-* headers, new error codes, cancellation-as-abort

  • Gather context (read changed files vs origin/v2/main)
  • Review core changes (fetchTracking redaction, createRemoteFetch)
  • Review new utils (mcpNetworkHeaders, correlateTransportErrors)
  • Review UI components (Network/Protocol entries, reveal flow)
  • Review test-server showcase changes
  • Post review feedback

This is a large, well-structured PR with strong test coverage, careful comments, and clean separation between the pure logic (mcpNetworkHeaders, correlateTransportErrors) and the presentational components. The redaction fix and the "derived view" (non-mutating enrichProtocolEntries) are both nicely done. A few things stand out below.


🟠 1. The reveal one-shot may cancel its own scroll (fragility / likely bug)

NetworkEntry.tsx:392-400

useEffect(() => {
  if (!revealed) return;
  setIsExpanded(true);
  const raf = requestAnimationFrame(() => {
    rootRef.current?.scrollIntoView({ behavior: "smooth", block: "center" });
  });
  onRevealComplete?.();          // ← clears the signal *synchronously*
  return () => cancelAnimationFrame(raf);
}, [revealed, onRevealComplete]);

In the real app, onRevealComplete is () => setRevealNetworkId(undefined) (InspectorView.tsx:1133). Calling it here flips revealNetworkId → undefined, which flips this entry's revealed prop true → false. On the resulting re-render the effect's cleanup runs and cancelAnimationFrame(raf) fires — racing the very rAF that performs the scroll. Whether the scroll survives depends on React's scheduling of the passive-effect-triggered re-render relative to the next animation frame, which is not something to rely on.

The unit test doesn't catch this because it passes a static revealed prop that never flips back (NetworkEntry.test.tsx:365-386), and onRevealComplete is a stable vi.fn() — so no cleanup runs and the rAF is never cancelled. The integration path where the signal clears isn't exercised.

Suggested fix — clear the signal inside the rAF, after the scroll, so the cleanup can no longer cancel a pending scroll (by the time onRevealComplete runs, the rAF has already fired and cancelAnimationFrame is a no-op):

const raf = requestAnimationFrame(() => {
  rootRef.current?.scrollIntoView({ behavior: "smooth", block: "center" });
  onRevealComplete?.();
});
return () => cancelAnimationFrame(raf);

Fix this →


🟡 2. Reveal fires twice (both the pinned column and the full-screen Network share revealId)

InspectorView.tsx:1122-1134 builds one networkScreenProps (with revealId) that is spread into both the embedded monitor instance (monitorScreens[NETWORK_TAB], line 1180) and the primary ScreenStage instance (line 1340). Since ScreenStage keeps inactive children mounted, when the sidebar is pinned two NetworkEntry instances match entry.id === revealId — both call scrollIntoView (the hidden one no-ops) and both call onRevealComplete. It works, but it's a slightly wasteful double-fire; harmless given the fix in #1 would keep the clear idempotent. Worth a one-line comment noting the double-mount is expected, or gating revealId to the currently-visible instance.


🟢 3. Minor doc drift after the Network → Protocol relocation

  • McpErrorBadge.tsx:24-25 — the doc comment still says "shown in the Network tab", but per this PR the chip now renders in the Protocol tab. Same for the COLOR_BY_CODE comment context. Quick comment refresh.
  • mcpNetworkHeaders.ts — module name/header comment frame it as "Network tab" logic, but classifyProtocolSpecError (used by ProtocolEntry) now lives here too. Acceptable (it's shared HTTP/spec logic), just slightly misleading given the relocation.

🟢 4. isCancellationAbort substring heuristic

mcpNetworkHeaders.ts:410-416 matches any entry.error containing "abort" or "cancel" (case-insensitive). This is a fetch/transport-level error string (not a tool result), so false positives are unlikely, and it's documented — flagging only for awareness. If the SDK ever surfaces a genuine transport failure whose message happens to contain "aborted", it would render as a neutral "Cancelled". Fine to leave as-is.


✅ Things done well

  • Redaction fix (fetchTracking.ts) — masking only string values of a sensitive field is the correct, minimal fix for the numeric error.code collision, and it's tested.
  • enrichProtocolEntries returns the same array reference when nothing matched — good for memoisation, and the "derived view, no mutation of MessageLogState" decision is the right call.
  • httpRelevant gating for the reveal link is a clean, well-justified rule with matching tests (link shown for thrown/header-tied, hidden for -32021/-32022).
  • The createRemoteFetch.ts note documenting the browser-path Mcp-Param-* gating is exactly the kind of hard-won finding worth writing down.

None of the above are blockers; #1 is the only one I'd want addressed (or explicitly reasoned about) before merge.
· v2/1628-network-modern-headers

…ment nits

Address @claude review of #1722:
- Reveal one-shot: move onRevealComplete() inside the rAF (after scrollIntoView)
  so clearing revealId (which flips revealed->false and runs the effect cleanup)
  can no longer race/cancel the pending scroll. Add a flip-back test.
- Comment the intentional double-mount of revealId (pinned monitor + full-screen
  ScreenStage both match; clear is idempotent).
- Doc drift after the Network->Protocol relocation: McpErrorBadge + the
  mcpNetworkHeaders module header now say the spec-error chip lives in Protocol.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

cliffhall commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

Thanks for the thorough review — addressed in 99aaf8a.

🟠 1. Reveal one-shot cancelling its own scroll — fixed. Good catch; the race was real. Moved onRevealComplete() inside the rAF, after scrollIntoView, so by the time the clear flips revealed → false and the effect cleanup runs, the frame has already fired and cancelAnimationFrame is a no-op. Added a NetworkEntry test that reproduces the integration path (a stateful harness whose onRevealComplete flips revealed back to false) and asserts the scroll still fires exactly once — the old ordering fails it. Also corrected the pre-existing test's stale "cleared synchronously" assertion to "cleared after the scroll".

🟡 2. Double-fire from the shared revealId — commented. It is intentional (pinned monitor instance + the always-mounted full-screen ScreenStage instance both match; the hidden one's scrollIntoView no-ops and the clear is idempotent). Left the single shared revealId and documented the expected double-mount at the networkScreenProps definition rather than gating per-instance — with bullet 1's idempotent clear it's harmless.

🟢 3. Doc drift after the Network → Protocol relocation — fixed. Updated McpErrorBadge.tsx (both the COLOR_BY_CODE and JSDoc "Network tab" → "Protocol tab") and reframed the mcpNetworkHeaders.ts module header to say it backs both tabs: the header helpers for Network, classifyProtocolSpecError for Protocol.

🟢 4. isCancellationAbort substring heuristic — leaving as-is per your note. It only ever sees fetch/transport-level error strings (never tool results), and the failure mode (a genuine transport error whose message contains "aborted" rendering as neutral "Cancelled") is benign and documented.

npm run ci green (validate, ≥90 coverage incl. integration, smoke, Storybook).

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 2m 30s —— View job


Review: Network modern Mcp-* headers, new error codes, cancellation-as-abort

  • Gather context (diff vs origin/v2/main)
  • Core changes (fetchTracking redaction, createRemoteFetch)
  • Pure logic (mcpNetworkHeaders, correlateTransportErrors)
  • UI components (Network/Protocol entries, reveal flow, App/InspectorView)
  • Test-server showcase
  • Post review

This is a large, cohesive PR and it's in good shape. The pure logic is cleanly separated from the presentational layer, the derived-view enrichment is well-reasoned, and the two concerns identified in the earlier review (the reveal one-shot cancelling its own scroll, the doc drift) are correctly resolved in 99aaf8a2. I re-read the whole diff fresh; a few small observations below, none blocking.


🟢 1. redactJsonValue leaks a non-string secret nested under an array (edge case)

core/mcp/fetchTracking.ts:124-127 — the fix to mask only string values of a sensitive key is correct and is the right call for the numeric error.code collision. One residual edge: a sensitive key whose value is an array of strings now recurses element-by-element, and the elements have no key context, so they aren't masked:

{ "password": ["s3cr3t", "other"] }   // → recursed, NOT masked

An object value ({ "token": { "access_token": "…" } }) is fine — it recurses and the inner string key still matches. Only the array-of-scalars-under-a-sensitive-key shape leaks, which no OAuth/token flow produces, so this is theoretical. Worth a one-line note in the comment (already thorough) that the masking assumes a sensitive field is a scalar or an object, not an array of scalars.


🟢 2. revealableIds is broader than the link gating (harmless redundancy)

revealableMessageIds (correlateTransportErrors.ts:102) returns every message id with a correlated fetch, not just spec-error entries — so a successful tools/call is "revealable". The actual link is still correctly gated twice downstream: ProtocolListPanel only wires onRevealInNetwork when revealableIds.has(id), and ProtocolEntry only renders the anchor inside McpSpecErrorAlert when specError.httpRelevant. So the broad set never produces a stray link. It reads a little surprising that the "revealable" set includes non-errors; a rename or a one-line comment ("superset — the spec-error/httpRelevant gate is applied at render") would save the next reader a double-take. Purely cosmetic.


🟢 3. Double-fire on reveal is now genuinely safe — confirmed

I traced the interaction the revealId-into-both-instances comment describes (InspectorView.tsx:1129-1138). With bullet 1's fix (clear inside the rAF), the flow holds up even when InspectorView re-renders mid-reveal under live traffic: a new onRevealComplete identity re-runs the effect, cleanup cancels the pending rAF and reschedules — it never cancels a fired one, so the scroll always lands and the clear is idempotent across both mounted instances. The comment accurately documents why the double-mount is expected. Good.


🟢 4. Nits already noted / accepted

  • isCancellationAbort substring heuristic (mcpNetworkHeaders.ts:413) — sees only transport-level error strings; the benign failure mode is documented. Fine as-is (already discussed in the prior round).
  • The response-headers HeadersTable correctly omits consistency (NetworkEntry.tsx:542), so a response header can never be false-flagged — the cross-check is request-only. 👍

✅ Things done well

  • Redaction fix — minimal, correct, and directly tested; the collision it fixes (numeric error.code vs. OAuth code) is exactly the kind of bug that only shows against real traffic.
  • enrichProtocolEntries — non-mutating derived view returning the same array reference when nothing matched is the right memoisation-friendly design, and the "delivered carries duration, enriched does not" tell is a nice touch.
  • httpRelevant gating for the reveal link — well-justified per-code table, matched by tests (shown for thrown -32601 / header-tied -32020, hidden for -32021/-32022).
  • createRemoteFetch.ts browser-path note — the hard-won Mcp-Param-* gating finding is exactly the kind of thing worth writing down, and the showcase server makes the whole taxonomy drivable live.
  • Coverage looks complete: new correlateTransportErrors, mcpNetworkHeaders, McpErrorBadge, and the reveal/force-expand/one-shot path all have tests, plus the redaction regression test.

Nothing here blocks merge — the three green items are polish/edge-case notes, not defects.
· v2/1628-network-modern-headers

@cliffhall cliffhall linked an issue Jul 18, 2026 that may be closed by this pull request
…rset

Address second @claude review of #1722 (both non-blocking polish notes):
- fetchTracking redaction: note that masking assumes a sensitive value is a
  scalar or object; an array-of-scalars under a sensitive key would slip through
  (no OAuth/token payload has that shape).
- revealableMessageIds: document it's a superset of the linked entries; the
  spec-error/httpRelevant narrowing is applied downstream at render.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks — both non-blocking polish notes applied in b80b744.

🟢 1. Redaction array-of-scalars edge — commented. Added a note that the masking assumes a sensitive value is a scalar or an object (recursed, so an inner sensitive string key is still masked), and that a sensitive key holding an array of scalars would slip through — no OAuth/token payload has that shape, so it's not handled. Left the logic as-is per your read.

🟢 2. revealableMessageIds superset — commented. Documented that it's deliberately a superset (any correlated message, including a successful tools/call), and that the narrowing is applied downstream at render: ProtocolListPanel only wires onRevealInNetwork for ids in the set, and ProtocolEntry only renders the anchor when the spec error is httpRelevant — so a non-error correlated message never yields a stray link. Kept the name (it accurately describes "has a correlated fetch"), just removed the double-take.

🟢 3 / 4. Double-fire safety confirmation and the accepted nits — no change needed; thanks for tracing the re-render-mid-reveal case.

npm run ci green (validate, ≥90 coverage incl. integration, smoke, Storybook).

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 2m 38s —— View job


Review: Network modern Mcp-* headers, new error codes, cancellation-as-abort

  • Gather context (diff vs origin/v2/main)
  • Core changes (fetchTracking redaction, createRemoteFetch)
  • Pure logic (mcpNetworkHeaders, correlateTransportErrors)
  • UI components (Network/Protocol entries, reveal flow, App/InspectorView)
  • Test-server showcase
  • Post review

I re-read the full diff fresh. The two earlier rounds' findings (the reveal one-shot cancelling its own scroll, the doc drift, the redaction array-of-scalars note, the revealableMessageIds superset comment) are all correctly resolved in 99aaf8a2 / b80b7449. The pure-logic/presentational split is clean, the derived-view enrichment is well-reasoned, and coverage is thorough. One new observation below that the prior rounds didn't surface — non-blocking, but worth a decision before merge.


🟡 1. classifyProtocolSpecError treats every in-band -32601 as the modern 404 taxonomy

mcpNetworkHeaders.ts:257-269 + ProtocolEntry.tsx:212-221

Unlike classifyMcpSpecError (which correctly gates -32601 on responseStatus === 404, per its own comment at line 231-234), the Protocol-side classifyProtocolSpecError recognises -32601 from the code alone, on purpose — the Protocol view has no HTTP status. The problem is that -32601 "Method not found" is the single most generic standard JSON-RPC error: any MCP server can return it in-band on HTTP 200 for an unsupported method (e.g. a client probing completion/complete or logging/setLevel against a server that doesn't advertise it). That ordinary error now renders, in the Protocol tab, as:

  1. the modern -32601 MethodNotFound spec badge (wide layout), and
  2. the alert with description "…A JSON-RPC error body on an HTTP 404 marks a modern server — a legacy HTTP+SSE server returns a bare 404 with no body." (SPEC_ERROR_META, line 199-207), and
  3. the "View the HTTP request in the Network tab →" link (httpRelevant: true), which then jumps to a plain HTTP 200 entry.

So a completely normal method-not-found is dressed up in modern-transport / 404 framing that doesn't apply to it. It's not a crash — the badge label ("MethodNotFound") is technically accurate and the reveal still correlates — but the description text is misleading for the 200 case, and the link claims diagnostic value ("the real 404 response only exists in the Network log", per the PR's own table) that isn't there.

The existing test only covers the folded/thrown 404 path (ProtocolEntry.test.tsx:563, and the enrichment fixture explicitly says "arrives as HTTP 404"); there's no case for a delivered in-band -32601 on a 200, so this framing mismatch isn't exercised.

Since the correlated fetch (and thus its responseStatus) is available at the App/InspectorView correlation layer, one option is to gate the Protocol -32601 classification to entries whose correlated fetch was actually a 404 — mirroring classifyMcpSpecError's guard — and let a plain in-band -32601 fall through to the ordinary error rendering. A lighter-touch alternative is to soften the -32601 description so it doesn't assert 404/modern-server semantics unconditionally. Either way, -32020/-32021/-32022 are SEP-reserved and don't have this ambiguity — only -32601 does.

Fix this →


🟢 2. Test-server injector reads req.body (confirm body-parse ordering)

test-server-http.ts:157-179specErrorInjector reads req.body and is mounted as app.post("/mcp", ...mcpMiddleware, ...extraMiddleware, route). This relies on a JSON body parser running earlier in mcpMiddleware (and on route, not mcpMiddleware, being the terminal MCP handler — otherwise the injector never runs). Both hold given the showcase demonstrably returns the crafted errors live, so this is just a note for the next reader: the injector's correctness is coupled to that middleware ordering. Test-only, no action needed.


🟢 3. Minor: SPEC_ERROR_TRIGGERS message vs. real spec-error semantics

test-server-http.ts:135-156 — the injected trigger_header_mismatch message ("Mcp-Method header does not match…") is a fixed string, which is exactly the point (it's a synthetic exerciser), but note it will never carry the Mcp-* header context a real -32020 would — so the Network tab's header/body consistency marker won't line up with the injected error. Fine for a showcase; just don't mistake the injected path for the organic -32020 the strict-modern get_weather path produces from the web client.


✅ Things done well

  • Redaction fix (fetchTracking.ts:124-133) — masking only string values of a sensitive key is the correct, minimal fix for the numeric error.code collision, and the assumption boundary (scalar/object vs. array-of-scalars) is now documented and tested.
  • enrichProtocolEntries — non-mutating derived view that returns the same array reference when nothing matched (correlateTransportErrors.ts:139,150); the App-level useMemo and the "delivered carries duration, enriched does not" tell are the right calls.
  • httpRelevant gating — the per-code table is well-justified and matched by tests (link shown for thrown -32601 / header-tied -32020, hidden for -32021/-32022). Finding Progress notifications #1 is the one seam in that model.
  • Double-fire safety on reveal is genuinely correct now (clear inside the rAF), and the InspectorView.tsx:1129-1138 comment documents why the double-mount is expected.
  • createRemoteFetch.ts browser-path note — the hard-won Mcp-Param-* gating finding is exactly the kind of thing worth writing down, and the showcase server makes the whole taxonomy drivable live.

Nothing here blocks merge — #1 is the only item I'd want an explicit decision on (gate to 404, or soften the copy); the other two are notes.
· v2/1628-network-modern-headers

Address third @claude review of #1722. classifyProtocolSpecError recognised
-32601 from the code alone, so an ordinary in-band 'method not found' returned
on HTTP 200 was dressed up as the modern MethodNotFound taxonomy: the spec
badge, a 404-asserting alert, and a 'reveal in Network' link that jumped to a
plain 200 (claiming diagnostic value that wasn't there).

Gate the modern -32601 treatment on the correlated fetch actually being a 404
(mirroring classifyMcpSpecError). -32020/-32021/-32022 are SEP-reserved and
unaffected. The correlated HTTP status is threaded from InspectorView (new
correlatedFetchStatusById) through ProtocolScreen/ProtocolListPanel to
ProtocolEntry; when unknown, -32601 falls back to code-only recognition (the
thrown-404 path always carries a 404). Badge, alert copy, and link now resolve
together, so a plain in-band -32601 falls through to ordinary error rendering.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks — the -32601 seam is a good find. Fixed in d767e7d with your preferred option (a): gate the modern treatment to a genuine 404.

🟡 1. classifyProtocolSpecError treating every in-band -32601 as the modern taxonomy — fixed.

  • classifyProtocolSpecError(code, data, httpStatus?) now only classifies -32601 as MethodNotFound when the correlated fetch was an actual 404 (mirroring classifyMcpSpecError's guard). -32020/-32021/-32022 are SEP-reserved and unaffected. When the status is unknown it falls back to code-only recognition — the thrown-404 path always carries a 404, so that only affects a -32601 with no HTTP record.
  • The correlated status is threaded from the App layer you pointed at: new correlatedFetchStatusById(messages, fetchEntries) in correlateTransportErrors.ts, memoised in InspectorView, passed correlatedStatusByIdProtocolScreenProtocolListPanel → per-row correlatedHttpStatusProtocolEntry.
  • This gates the whole modern treatment together (badge, alert copy, and the reveal link all key off the same classification), so a plain in-band -32601 on a 200 now falls through to ordinary error rendering — no modern badge, no 404-asserting description, no link to a 200. The real thrown-404 case keeps its precise description.
  • Tests: classifyProtocolSpecError 404-vs-200 gating + the 400-status errors staying unaffected; correlatedFetchStatusById (mapping, skips non-transport/statusless/uncorrelated, last-wins on repeated id); and a ProtocolEntry case asserting an in-band -32601 on a 200 shows no chip/alert/link but still renders as an ordinary Error.

🟢 2 / 3. Test-server injector notes — acknowledged, no change. The specErrorInjector's coupling to the JSON-body-parse / terminal-handler ordering, and the synthetic trigger_header_mismatch message not carrying real Mcp-* header context, are both intended for the showcase exerciser (distinct from the organic strict-modern get_weather path from the web client). Left as-is; thanks for flagging for the next reader.

npm run ci green (validate, ≥90 coverage incl. integration, smoke, Storybook).

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 2m 55s —— View job


Review: Network modern Mcp-* headers, new error codes, cancellation-as-abort

  • Gather context (diff vs origin/v2/main)
  • Review latest fix (d767e7d4-32601 404 gating)
  • Re-check core changes (fetchTracking, createRemoteFetch)
  • Re-check pure logic (mcpNetworkHeaders, correlateTransportErrors)
  • Re-check UI components (Network/Protocol entries, reveal flow)
  • Post review

I re-read the full diff after d767e7d4. The threading of the correlated HTTP status (correlatedFetchStatusByIdProtocolScreenProtocolListPanel → per-row correlatedHttpStatusProtocolEntry) is clean and per-row, and gating badge + alert copy + link together on the same classification is the right call. The three prior rounds' findings are all correctly resolved. One new observation on the -32601 gate — the fix closed the 200 case but the unknown-status escape hatch reopens the same seam for the case that matters most in practice. Non-blocking, but worth a decision.


🟡 1. The -32601 unknown-status fallback re-admits ordinary/stdio errors into the modern framing

mcpNetworkHeaders.ts:270-276

if (
  code === ProtocolErrorCode.MethodNotFound &&
  httpStatus !== undefined &&      // ← escape hatch
  httpStatus !== 404
) {
  return null;
}

The 404 gate only bites when a status is known. When httpStatus === undefined it falls through to code-only recognition — and the test at mcpNetworkHeaders.test.ts:203-204 codifies that (classifyProtocolSpecError(-32601)MethodNotFound). The comment justifies this with "the thrown-404 path always carries a 404, so this only affects a -32601 with no correlated HTTP record." That's true, but the conclusion is effectively inverted — trace which entries actually reach the undefined branch:

  • A thrown/folded -32601 only exists in the Protocol log because enrichProtocolEntries found a fetch entry with a JSON-RPC error body. That same fetch entry carries responseStatus: 404, so correlatedFetchStatusById returns 404 for it. The thrown path therefore always resolves to httpStatus === 404 and never reaches the undefined branch.
  • So the undefined branch is reached only by an ordinary in-band -32601 with no correlated 404 — most commonly a stdio connection (the Network tab is HTTP-only, so network is empty → status map empty → correlatedHttpStatus undefined), or an HTTP delivered-in-band -32601 whose fetch record was rotated out (Network log: response body silently dropped when entry rotates out before fetchRequestBodyUpdate arrives #1390).

In other words, the fallback exclusively serves the exact case the fix set out to exclude. Over stdio, an unsupported-method probe now renders with:

  1. the -32601 MethodNotFound modern badge, and
  2. the alert copy "A JSON-RPC error body on an HTTP 404 marks a modern server — a legacy HTTP+SSE server returns a bare 404 with no body" (SPEC_ERROR_META, mcpNetworkHeaders.ts:200-207) — which is flatly wrong for a transport that has no HTTP at all.

(The reveal link doesn't appear in these cases — revealableMessageIds requires a correlated fetch — so it's badge + misleading copy, not a stray link. -32020/-32021/-32022 are SEP-reserved and unambiguous, so only -32601 has this.)

Suggested fix — drop the escape hatch so -32601 requires a genuine 404 (the thrown path always carries one, so nothing intended is lost):

if (code === ProtocolErrorCode.MethodNotFound && httpStatus !== 404) {
  return null;
}

Then update the mcpNetworkHeaders.test.ts:203 case (the -32601-classified-by-code-alone assertion) to expect null, and ideally add an explicit "stdio / no correlated fetch → ordinary error, no modern badge" case at the ProtocolEntry level. The HEADER_MISMATCH_ERROR_CODE half of that test (line 205-207) stays valid.

Fix this →


🟢 2. Notes (no action)

  • correlatedFetchStatusById last-wins on repeated ids (correlateTransportErrors.ts:150-157) — consistent with indexTransportFetchErrors and correlateFetchEntry, which also take the last match. Good that the three stay aligned; a retry that ends 200 after a 404 would flip the classification, but that's the correct "most-recent-truth" behavior and matches the reveal target.
  • Reveal double-fire safety — with the clear moved inside the rAF (NetworkEntry.tsx:399-403), the pinned double-mount and a re-render-mid-reveal are both genuinely safe; the InspectorView.tsx:1141-1148 comment documents why. Confirmed.
  • Redaction fix (fetchTracking.ts:118-133) — masking only string values of a sensitive key is the correct minimal fix for the numeric error.code collision, and the array-of-scalars boundary is now documented.

✅ Overall

This is a large, cohesive, well-tested PR with a clean pure-logic/presentational split and careful comments. The derived-view enrichment (non-mutating, referentially stable), the httpRelevant link gating, and the createRemoteFetch.ts browser-path Mcp-Param-* finding are all strong. Nothing blocks merge — #1 is the one item I'd want an explicit decision on (require 404, or keep the fallback and soften the stdio-facing copy); everything else is confirmation of prior rounds.
· v2/1628-network-modern-headers

…s fallback)

Address fourth @claude review of #1722. The prior fix only rejected -32601 when
the status was *known* and not 404; an unknown status fell back to code-only
recognition. But the thrown-404 path always carries a 404 (folded in from a 404
fetch), so the unknown-status branch is only ever reached by an ordinary in-band
-32601 with no correlated 404 — most notably a stdio connection (no HTTP at all)
— which then wrongly got the modern MethodNotFound badge and 404-asserting copy.

Drop the escape hatch: -32601 is the modern taxonomy only on httpStatus === 404.
Nothing intended is lost (the genuine case always has its 404). Update the tests
(code-only/undefined -32601 now expects null; add a stdio no-correlated-fetch
ProtocolEntry case) and mark the MethodNotFound story with correlatedHttpStatus
404.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Great catch — the escape hatch was inverted logic. Fixed in 729e056 by requiring a genuine 404.

🟡 1. -32601 unknown-status fallback re-admitting ordinary/stdio errors — fixed. You're exactly right: the thrown/folded -32601 always resolves to httpStatus === 404 (it only reaches the Protocol log because enrichProtocolEntries found a 404 fetch, which correlatedFetchStatusById reports as 404), so the undefined branch was reached only by an ordinary in-band -32601 with no correlated 404 — most importantly a stdio connection, where network is empty. Dressing that up with the modern badge + the "…on an HTTP 404…" copy was flatly wrong for a transport with no HTTP.

Dropped the escape hatch — the guard is now simply:

if (code === ProtocolErrorCode.MethodNotFound && httpStatus !== 404) return null;

Nothing intended is lost (the genuine modern case always carries its 404). Updated the doc comment to explain why unknown-status is deliberately not treated as a 404, and the tests: classifyProtocolSpecError(-32601) / (-32601, undefined, undefined) now expect null; added a ProtocolEntry "stdio / no correlated fetch → ordinary error, no modern badge" case; and marked the MethodNotFound story with correlatedHttpStatus: 404 (it's the genuine thrown-404). The -32020/-32021/-32022 code-only path stays valid.

🟢 2. Notes — acknowledged, no action. Last-wins on repeated ids (aligned with indexTransportFetchErrors / correlateFetchEntry), reveal double-fire safety, and the redaction fix — all confirmations of prior rounds. Thanks.

npm run ci green (validate, ≥90 coverage incl. integration, smoke, Storybook).

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 1m 13s —— View job


Review: Network modern Mcp-* headers, new error codes, cancellation-as-abort

  • Gather context (diff vs origin/v2/main)
  • Re-check the -32601 404 gate (729e056c) and its tests
  • Core changes (fetchTracking redaction, createRemoteFetch)
  • Pure logic (mcpNetworkHeaders, correlateTransportErrors)
  • UI components (Protocol/Network entries, reveal flow, InspectorView)
  • Test-server showcase
  • Post review

I re-read the full diff fresh after 729e056c. This one is ready — every finding from the four prior rounds is correctly resolved, and I didn't surface anything new that blocks merge. Notes below are confirmations plus one tiny cosmetic observation.


✅ The -32601 404 gate (729e056c) is correct

classifyProtocolSpecError (mcpNetworkHeaders.ts:273) is now the clean:

if (code === ProtocolErrorCode.MethodNotFound && httpStatus !== 404) return null;

I traced the reachability argument end to end and it holds:

  • The genuine modern case is thrown → folded by enrichProtocolEntries only because a fetch with a 404 error body exists → correlatedFetchStatusById reports 404 → classified. ✔
  • An ordinary in-band -32601 on a 200, or a stdio -32601 (no Network entry → no correlated status → undefined), now falls through to ordinary error rendering — no modern badge, no 404-asserting alert copy, and no link (the httpRelevant gate at ProtocolEntry.tsx:433-434 never fires because specError is null). ✔

The doc comment at mcpNetworkHeaders.ts:249-265 accurately explains why undefined is deliberately not treated as 404, and the tests cover the 404/200/undefined split plus the stdio-no-correlated-fetch ProtocolEntry case. This closes the seam cleanly.


🟢 One cosmetic note (no action needed)

classifyProtocolSpecError recognises -32020/-32021/-32022 from the code alone (correct — SEP-reserved), and only -32601 consults httpStatus. For a delivered -32020 HeaderMismatch (httpRelevant: true, HTTP 400) the reveal link still works because the delivered 400 has a correlated transport fetch, so revealableMessageIds includes it and the anchor renders. I confirmed that path is intact — just flagging that the "link only for thrown-or-header-tied" rule now correctly spans both a thrown 404 (-32601) and a delivered 400 (-32020), which the per-code httpRelevant table already encodes. Nothing to change.


✅ Confirmations from prior rounds (all still good)

  • Redaction fix (fetchTracking.ts:110-138) — masks only string values of a sensitive key; the numeric error.code collision is gone and the array-of-scalars boundary is documented. Directly tested.
  • Reveal one-shot (NetworkEntry.tsx:396-404) — onRevealComplete() is inside the rAF after scrollIntoView, so cleanup can only cancel a pending frame, never a fired one. Double-mount (pinned + full-screen ScreenStage) is idempotent and documented at InspectorView.tsx.
  • enrichProtocolEntries — non-mutating derived view, same array reference when nothing matched; the "delivered carries duration, enriched doesn't" tell is a nice touch.
  • correlatedFetchStatusById — last-wins on repeated ids, consistent with indexTransportFetchErrors / correlateFetchEntry; threaded per-row through ProtocolScreen → ProtocolListPanel → ProtocolEntry.
  • createRemoteFetch.ts browser-path Mcp-Param-* note and the live-drivable showcase server remain the standout artifacts of this PR.

Coverage looks complete across the new pure logic, the reveal path, and the redaction regression. Nothing blocks merge — 🚢.
· v2/1628-network-modern-headers

@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the thorough five-round pass — 🚢 confirmed, no actionable items remaining. Summary of what the review surfaced and how it landed:

  1. Reveal one-shot cancelling its own scroll (🟠) → moved onRevealComplete() inside the rAF; added a flip-back test.
  2. Doc drift after the Network→Protocol relocation (🟢) → McpErrorBadge + the mcpNetworkHeaders module header corrected.
  3. Redaction array-of-scalars edge / revealableMessageIds superset (🟢) → documented both boundaries.
  4. classifyProtocolSpecError treating every in-band -32601 as the modern 404 taxonomy (🟡) → gated to a genuine 404 via a per-row correlated HTTP status threaded from InspectorView (correlatedFetchStatusById) down to ProtocolEntry.
  5. The -32601 unknown-status escape hatch re-admitting stdio/in-band errors (🟡) → dropped the fallback; -32601 is the modern taxonomy only on httpStatus === 404.

All commits green under npm run ci (validate, ≥90 per-file coverage incl. the web integration project, smoke, Storybook).

@cliffhall
cliffhall merged commit d552564 into v2/main Jul 18, 2026
3 checks passed
@cliffhall
cliffhall deleted the v2/1628-network-modern-headers branch July 18, 2026 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Monitoring: Mcp-* headers, new error codes, cancellation-as-abort

1 participant