Refactor MCP HTTP proxy into kind-specific implementations#1977
Open
jfallows wants to merge 28 commits into
Open
Refactor MCP HTTP proxy into kind-specific implementations#1977jfallows wants to merge 28 commits into
jfallows wants to merge 28 commits into
Conversation
…to per-request-kind stream classes Replace the single general-purpose McpProxy/HttpProxy pair — which branched on the McpBeginEx kind throughout handleRequest and onUpstreamResponse — with dedicated single-purpose stream classes, one per MCP method, mirroring the per-method stream pattern in binding-mcp's McpClientFactory. - abstract McpProxy: shared MCP-side stream plumbing (frame dispatch, reply writers, decode/encode slots, cleanup); per-kind action via onMcpRequest - abstract McpHttpProxy: the two kinds that proxy to an upstream HTTP endpoint (tools/call, resources/read) — owns the paired HttpProxy, the request-shaping path built from route.with, and the streaming request/response machinery; per-kind response mapping via onUpstreamResponse - McpToolsCallProxy, McpResourcesReadProxy extend McpHttpProxy - McpToolsListProxy, McpResourcesListProxy, McpPromptsListProxy, McpPromptsGetProxy extend McpProxy - newStream dispatches to the per-kind class; HttpProxy is generic again, with kind checks replaced by polymorphic hooks (responseStreamable, tool) Behavior-preserving: all 29 McpHttpProxyIT tests pass, plus unit tests and checkstyle. Keeps the per-side decodeSlot/encodeSlot model from #1975. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…de onMcpBegin per kind Follow the http-kafka proxy pattern where each concrete stream owns its frame handler rather than a shared base method delegating to an *Impl hook. The base McpProxy.onMcpBegin handles the buffered kinds; McpHttpProxy and McpToolsCallProxy override onMcpBegin directly. Cross-side callbacks (onUpstreamResponse, responseBegin, responseStreamable) remain abstract/overridable on the base, matching http-kafka's abstract onKafkaBegin. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…-issue-1976-ppliuw
…ly/doEncodeRequestBody The two methods that append bytes into the per-stream encodeSlot ahead of flushReply/flushRequestStream draining them as DATA frames were named stage/stageBytes, an invented verb with no precedent elsewhere in the runtime tree. Follow the doEncode<Thing> shape used by binding-mqtt/binding-http (doEncodePublishV4, doEncodeConnack, ...) for methods that assemble bytes destined for the wire: rename to doEncodeReply (McpProxy, overloaded for byte[] and DirectBufferEx) and doEncodeRequestBody (HttpProxy, was stageRequestBody). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…ProxyFactory Following an audit of the stream classes for heap allocations on the data path (per runtime/AGENTS.md), fix two real, contained findings: - doMcpReply(String) re-encoded the cached tools/list, resources/list, prompts/list JSON String into a fresh byte[] via getBytes(UTF_8) on every single request, despite the JSON text itself being memoized. Cache the encoded byte[] directly on McpHttpBindingConfig instead of the String, and retype doMcpReply to take byte[] and call doEncodeReply(byte[]) directly (no other caller used the String overload). - Four computeIfAbsent(schema, this::newXxx) call sites reallocated a fresh capturing method-reference object on every call, cache hit or miss, even though the JsonPipeline values themselves were correctly cached. Hoist the four method references into reusable Function fields. Two other candidates from the audit (doHttpBegin's per-header/per-credential lambda captures, onHttpBegin's .asString() header matching) were confirmed via cross-codebase grep to be the standard Zilla-wide idiom (HttpKafkaProxyFactory, McpServerFactory, McpClientFactory, McpBindingConfig all use the same patterns) and left unchanged. A larger rework of the request/response templating chain (interpolate, encode, navigateBytes, queryStringFromBytes) is a real but separate, larger follow-up, deliberately out of scope here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
Two independently-designed investigations of the request/response templating chain (interpolate, encode, navigateBytes, queryStringFromBytes) converged on the same low-risk subset worth fixing now, while flagging that a full buffer-native rewrite of the whole chain carries real new risk in this codebase (production builds run with Agrona bounds-checking disabled, so a fixed-capacity buffer write without an explicit, tested overflow guard could silently corrupt memory instead of failing cleanly) and only a modest, once-per-request payoff — deferred as a separate, test-first follow-up. - encode(String): always paid 3 allocations (StringBuilder, getBytes(UTF_8), final toString()) to percent-encode a URL path/query value. Add an ASCII fast path: pre-scan for any char >= 0x80: for pure-ASCII input (the common case for tool args, ids, route params), iterate charAt(i) directly through the same percent-encode branching, skipping the UTF-8 byte conversion entirely (a single-byte ASCII char and its UTF-8 byte are bit-identical, so output is byte-for-byte unchanged). Non-ASCII input falls through to the original, unmodified byte-based path. - argReferences(route.with.headers.get(HEADER_PATH)): re-parsed a route's :path template into a fresh ArrayList + per-match substrings on every large-body-streaming tools/call stream open, despite depending only on immutable route config. Cache the result per route via the same IdentityHashMap + hoisted-Function computeIfAbsent pattern already used for templates/projectors/validators in this file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…-issue-1976-ppliuw
Audited every boolean field in McpHttpProxyFactory's stream classes against McpHttpState and the reference McpClientFactory implementation. Mixing mode/latch booleans alongside a state bitmask is the established Zilla convention (McpClientFactory itself has a dozen+ such fields) — not something to collapse into McpHttpState wholesale. But four fields turned out to duplicate information already tracked elsewhere, set only in the same synchronous call as the original with no independent transition: - McpToolsCallProxy.requestEndedMcp duplicated McpHttpState.initialClosed (state), set on the same call in onMcpEnd. - HttpProxy.responseEnded duplicated McpHttpState.replyClosed(state), set on the same call in onHttpEnd. - HttpProxy.responseStreaming duplicated McpHttpProxy's own responseStreaming field one-for-one, always toggled in lockstep since responseBegin() sets it and HttpProxy immediately mirrors it. HttpProxy already reads several of the server's fields directly elsewhere (requestStreaming, requestProjected, responseDone) rather than keeping local copies — this makes responseStreaming consistent with that. - McpToolsCallProxy.requestBegun duplicated McpHttpState.initialOpening(delegate.state): sendRequestBegin() calls delegate.doHttpBegin(...), which unconditionally sets that bit on the delegate; requestBegun was set true immediately after, in the same call. 13 booleans -> 9. No new state bits, no McpHttpState.java changes, no behavior change — purely reading existing state instead of a shadow copy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…large request bodies pumpRequest's STARVED handling discarded the whole decode window (decodeSlotOffset = 0) instead of keeping the partial trailing unit that JsonPipeline.remaining() reports as unconsumed, corrupting any multibyte UTF-8 character split across a DATA frame boundary in a streamed tools/call request body. Fix it to compact via remaining(), mirroring pumpResponse's already-correct implementation. Also widen the streaming path to cover unknown-length request bodies (contentLength < 0), not just ones known upfront to exceed the buffer slot, since a missing content-length header previously fell through to the fully buffered path and aborted once the body exceeded slot capacity. Adds a create.pr.fragmented spec scenario that splits a 2-byte UTF-8 character exactly across two DATA frames, reproducing the corruption before the fix and passing after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…single streaming pipeline Remove the content-length-based split between a fully-buffered onMcpRequest path and the incremental JsonPipeline path for tools/call requests with a body: every route with route.with.body configured now always builds and drives the same requestPipeline/pumpRequest machinery, regardless of size, so a small single-frame request is just the one-transform()-call special case of the same code a large streamed body runs through. unsatisfiedAccessors is a purely static, route-level check, so it's now computed once at onMcpBegin and applied on the first pumpRequest call (deferred past window-granting, since resetting immediately would close the stream before a client write already in flight can land). The tool.input schema validator is composed into the same pipeline (mirroring the validator+projector composition already used for upstream response validation) but only when the request is guaranteed to arrive within one bounded decode slot (contentLength known and <= slot capacity): JsonSchema's validator implementation requires full reassembly of any scalar value that spans multiple windows before validating it, which is incompatible with an unbounded streamed body and a fixed-size decode slot. pumpRequest's REJECTED handling now aborts the (possibly not-yet-opened) upstream HTTP connection and replies with the same graceful tool error the buffered path always used, rather than a raw reset/abort combo, since a connection may already be in flight by the time a validation failure is known. Adds a create.pr.malformed.request spec scenario covering a genuinely incomplete (not just schema-invalid) request body, which previously hit no test coverage on this path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…SET, not a synthesized reply
Align invalid-arguments handling with how the reference binding-mcp treats
the same scenario: McpProxyItemFactory.validateArgs resets the upstream leg
with a McpResetExFW carrying JSON-RPC code -32602 ("Invalid params") and
aborts downstream, leaving the terminal, wire-owning binding (McpServerFactory)
responsible for turning that into a graceful client-facing reply. This
binding sits in the same internal-proxy-hop position, so both pumpRequest's
REJECTED handling and onMcpRequest's pre-existing validate() failure branch
now do the same doMcpReset(-32602)+abort instead of synthesizing an
isError:true tool result themselves — removing an inconsistency that
predated this session (onMcpRequest) and one introduced by it (pumpRequest).
Also drops two per-stream boolean/field additions from the previous commit
(requestRejected, unresolvedAccessor) in favor of the existing McpHttpState
bitmask: doMcpReset already transitions state to closedInitial internally,
so pumpRequest's post-loop guards read McpHttpState.initialClosed(state)
directly instead of tracking a parallel flag, and the unsatisfied-accessors
check is now a plain guard clause backed by the already-memoized
unsatisfiedAccessors(route) lookup rather than a precomputed field.
Corrects the validator-gating comment to state the real constraint precisely
(an individual value must fit within one window, not the whole request) and
files #2016 against common-json for the underlying gap,
confirmed by direct code reading: a scalar value that never fits any window
causes the validator's pipeline to stall in STARVED forever, never resolving
to REJECTED even when the final window is marked last.
Updates create.pr.invalid and create.pr.malformed.request to expect the
resulting abort instead of a graceful reply, and adds search.code.invalid to
cover the onMcpRequest branch's updated behavior, previously untested.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…e, remove redundant booleans, hoist allocations Replace the requestEnd/replyComplete/responseStreaming per-stream booleans with McpHttpState CLOSING bits (reusing bit constants already declared but unused), matching the state-tracking convention used by McpClientFactory, HttpKafkaProxyFactory, and GrpcKafkaProxyFactory. responseStreaming was provably redundant with responsePipeline != null and is removed outright. Also hoist a handful of hot-path allocations that were previously reallocated on every call: McpHttpBindingConfig's un-hoisted this::resolveJsonSchema method reference, the four Map.of(JsonSink.DELIVERY, ...) sink-config literals in McpHttpProxyFactory, and unsatisfiedAccessors' unconditional ArrayList allocation (now lazily allocated only when an unsatisfied accessor is actually found).
…directly into encodeSlot buildToolError/buildResource/buildPromptGet and doMcpToolSuccess staged the full reply envelope in a shared scratch buffer before a single copy into encodeSlot. doMcpToolSuccess already proved that multiple ordered doEncodeReply calls straight into encodeSlot work safely with no extra DATA-frame fragmentation, so the other three builders now follow the same shape via two new primitives (doEncodeReplyJsonString, for escaped-string and buffer-splice writes) and a shared completeReply() tail. This removes the replyBuffer field and its backing allocation entirely, along with the now-unused doMcpReplyBytes.
…ResourceRoute Replaces stream().filter().findFirst() with a plain for-loop, matching the style already used by resolveResource/toolGuarded, removing a per-request Stream pipeline and capturing lambda allocation on the tools/call and resources/read hot paths.
…nce bindings Renames onUpstreamResponse/onUpstreamAbort to onHttpResponse/onHttpAbort (keeping the protocol prefix on abstract hooks, matching HttpKafkaProxyFactory's onKafkaBegin), grantHttpReply to grantHttpReplyWindow, responseComplete to completeResponse, flushRequestStream to flushRequest, and HttpProxy's server field to mcp (avoiding collision with XxxServerFactory terminology). Also reorders HttpProxy's methods so on* handlers precede do*/helper methods, matching McpProxy's own ordering and the three reference bindings. No behavior change.
… aliasing hazard DefaultBufferPool.buffer(slot) returns one shared, mutable slotBuffer field re-wrapped in place on every call, not a fresh view per slot. With a single bufferPool field serving both HttpProxy's decode/encode slots and McpProxy's, a DirectBufferEx reference obtained from one slot silently repoints to whatever slot the next buffer() call touches, regardless of which object holds the reference. This previously blocked eliminating the text()/cap() String round-trip in the tools/call non-2xx error path: a prior attempt at a zero-copy encode splice read back its own just-written prefix bytes instead of the real upstream response body once encodeSlot's buffer() call repointed the same wrapper backing the decode-side body reference. Splits the single bufferPool into decodePool/encodePool via duplicate(), matching the exact pattern McpClientFactory already uses (decodePool = context.bufferPool(), encodePool = context.bufferPool().duplicate()) so concurrent access to a decode slot and an encode slot always resolves through distinct DefaultBufferPool instances with independent slotBuffer wrappers. With that in place, doEncodeToolError can safely splice the upstream response body straight into encodeSlot without any intermediate copy; text()/cap() are removed as they become unused.
…s on buffered resources/read Two proxy pass-through bugs, fixed test-first: 1. An MCP client resetting the reply direction of an in-flight tools/call or resources/read stream never tore down the paired upstream HTTP connection. onMcpReset is made overridable and McpHttpProxy now mirrors it to the delegate via a new HttpProxy.doHttpReset, which sends RESET on the delegate's own reply direction (not ABORT) -- propagating RESET as RESET, matching the existing ABORT-as-ABORT mirroring already used elsewhere in this class. Guarded the same way doHttpAbort already is, so it correctly no-ops if the upstream request was never opened. 2. McpResourcesReadProxy's buffered onHttpResponse never checked the upstream HTTP status before treating the response as successful content, unlike McpToolsCallProxy's equivalent method. A non-2xx response with a schema-satisfying body was reported to the MCP client as a successful resources/read. Now aborts on non-2xx, consistent with the existing produced<0 handling for this proxy kind. The streaming resources/read path has the same gap in principle but is left unaddressed -- responseBegin has no status available to check before committing to a reply -- and is flagged with a comment. New tests: shouldAbortUpstreamWhenReplyReset, shouldAbortResourceReadWhenUpstreamStatusNotOk. Fixing bug 1 also surfaced a pre-existing k3po script race: "read abort" in this test harness is both a passive matcher and an active reset fallback, so any script ending on an unsynchronized "read abort" right after writing a request body could race the request/response delivery once onMcpReset became consequential. Added explicit notify/await synchronization to the two existing scenarios affected (create.pr.aborted, read.order.malformed) alongside the two new ones.
Removes the buffered onHttpResponse path entirely: tools/call and resources/read now open the reply and drive responsePipeline as soon as status/content-type are known, generating directly against encodeSlot's live buffer instead of a bounded scratch region. Structured content streams first; tool.summary is resolved from values McpHttpResults captures as the response passes through (a response-side counterpart to McpHttpArguments), so the reply envelope no longer needs the whole body buffered up front. Non-2xx tools/call responses stream back as escaped text via a dedicated error-relay path with no size cap. Also adds the McpServerIT/HttpClientIT peer-to-peer spec coverage for the two new large-response scenarios, matching this repo's test-first convention for every new spec scenario. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…-issue-1976-ppliuw
Replaces the post-hoc, unbounded raw-byte write of a tools/call success
envelope's trailer (content/isError, including the ${result.*}-interpolated
summary) with McpHttpToolResult, a JsonTransform that wraps the whole
envelope as generator-tracked events sharing structuredContent's own bounded,
resumable write path. A summary interpolating a large captured value could
previously overflow encodeSlot once structuredContent had already filled it,
since the trailer bypassed the pipeline's own backpressure entirely.
A suspended synthetic write now resumes via sink.resume(...) rather than
re-invoking sink.transform(...), avoiding the double-emission bug present in
the existing McpSchemeInjector precedent (harmless there only because its
injected content is always small).
The two remaining fixed-size suffixes (tools/call error-relay, resources/read)
now own their own bounds-safety via a small ensureEncodeRoom helper instead of
relying on the caller to reserve headroom.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…d fix common-json SUSPENDED/COMPLETED bug McpHttpResults blindly forwarded its upstream JsonController to its downstream sink, so a byte-preferring terminal sink's segmentable() request reached the real parser unblocked. Once granted, the whole document streamed as one opaque SEGMENT run and the KEY_NAME events McpHttpResults matches paths against never fired, silently breaking tool.summary result-path capture. Fixed by giving McpHttpResults its own downstream JsonController that declines segmentable() while paths are configured (forwarding it only when there is nothing to watch), forwards verbatim(), and adds matching resume()/flush() overrides, per the same mediating-transform rule common-json's JsonSchemaImpl.Validator already follows. Chasing this down through the real engine (not just unit tests) surfaced a second, independent bug in common-json: JsonSchemaImpl.Validator.verdictStatus checked a VALID schema verdict before checking a SUSPENDED downstream status, so a mediating stage that injects more content once a validated value closes (e.g. McpHttpToolResult wrapping structuredContent in a larger envelope) could have its own SUSPENDED silently reported as COMPLETED, truncating everything still queued downstream. Fixed the precedence and added a regression test reproducing the exact failure mode with a minimal injector stage. Also: - Add k3po IT coverage for three tool-response shapes that previously had no test coverage at all (only exercised by since-removed unit tests): a tool with no configured summary, an array-rooted output schema, and a bare-scalar-rooted output schema (ping/list_tags/count_items). - Add the get.report.large scenario proving tool.summary interpolation of a captured value survives suspend/resume across the encode slot when the captured value is itself large. - Remove McpHttpResultsTest.java/McpHttpToolResultTest.java: this module has no precedent for unit-testing transform/ classes, and per AGENTS.md this repo verifies streaming/protocol behavior via k3po IT specs, not JUnit. - Correct a misleading example in AGENTS.md that showed running an *IT.java class via `mvnw test`, which silently skips k3po startup and produces a misleading "Is K3PO ready?" failure instead of a real one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…allback McpHttpArguments (request-side arg capture) had the same mediating-transform bugs already fixed in McpHttpResults on the response side: a top-level argument value spanning multiple decode windows was truncated instead of accumulated, and segmentable()/verbatim()/consumed() weren't properly intercepted and forwarded, risking the whole request going opaque before KEY_NAME events could ever be matched. Fixed by mirroring McpHttpResults' already-shipped shape, proven by a new echo_id tool/route whose argument spans multiple windows. Separately, the response-streaming rewrite regressed the no-summary content[0].text fallback: it now emits an empty string instead of the pre-existing behavior of stringifying structuredContent as text. Restored via a new McpHttpResultText transform stage that mirrors the response's canonical JSON into a bounded, dedicated buffer as it streams (abandoning gracefully rather than overflowing on a pathologically large response), proven by a new get_profile tool/scenario with nested object/array/boolean/ null content. Also fixes binding-mcp-openapi's create_pr scenarios, which depend on this fallback and were failing in CI against the regression.
…-issue-1976-ppliuw # Conflicts: # specs/binding-mcp-openapi.spec/src/main/scripts/io/aklivity/zilla/specs/binding/mcp/openapi/streams/mcp/create.pr.10k/client.rpt # specs/binding-mcp-openapi.spec/src/main/scripts/io/aklivity/zilla/specs/binding/mcp/openapi/streams/mcp/create.pr.10k/server.rpt # specs/binding-mcp-openapi.spec/src/main/scripts/io/aklivity/zilla/specs/binding/mcp/openapi/streams/mcp/create.pr/client.rpt # specs/binding-mcp-openapi.spec/src/main/scripts/io/aklivity/zilla/specs/binding/mcp/openapi/streams/mcp/create.pr/server.rpt
…ry text buffer
The streaming/buffer audit found McpHttpResultText's dedicated buffer was
the only remaining per-stream buffer beyond decode/encode slot, needed only
to reconstruct content[0].text when a tool had no summary configured.
Making summary required removes that fallback path entirely: every tool's
content[0].text is now always the interpolated summary via the existing,
lightweight McpHttpResults capture, with no separate document buffer.
binding-mcp-openapi's composite generator now defaults a tool's summary to
a literal "Call <operationId>" (computed at generation time, not a
${...} template) when the underlying OpenAPI operation has none, since
OpenAPI's summary field is itself optional.
…r.aborted Mirrors the same notify/await synchronization already applied to binding-mcp-http.spec's copy of this scenario earlier this session: k3po's "read abort" is both a passive matcher and an active abort fallback, so an unsynchronized read abort right after a write can race the request delivery itself. This alone does not yet make shouldAbortToolCreatePr pass — it surfaces a deeper, separate issue in the mcp_openapi-to-mcp_http relay, under active investigation.
… opened McpOpenapiClientFactory's doMcpOpenapiEnd/doMcpOpenapiAbort silently dropped the signal when the reply direction hadn't sent a BEGIN yet (e.g. the upstream aborts before mcp_http0 ever produces a response), leaving the real MCP client hanging with no reply frame at all. Open the reply first, mirroring the existing doHttpBegin-before-abort pattern already used elsewhere in this codebase, so END/ABORT are always deliverable. Also guard doMcpOpenapiBegin so it only opens the reply once.
…-response
McpHttpProxy.onHttpAbort tore down the response (cleanupResponse) and
signaled the mcp reply direction (doMcpAbort) but never released encodeSlot,
so any response mid-construction when the upstream aborts (error-relay's
TOOL_ERROR_PREFIX, or a partially streamed success envelope) leaked that
buffer for the lifetime of the worker. The parallel onHttpEnd path already
drains and releases it correctly via pumpResponse's terminal-status
handling; onHttpAbort short-circuits past that, so it must release the slot
directly.
This is the confirmed root cause of the intermittent
McpOpenapiClientIT#shouldAbortToolCreatePr failures ("Some resources not
released: 1 buffers" / "close timeout") — traced via direct acquire/release
instrumentation showing the mcp-side reply encodeSlot acquired during
error-relay setup with no matching release once the upstream aborted before
ever completing a response. Verified 27/27 across three batches with the
fix applied, versus frequent failures before it.
Adds a dedicated regression scenario (create.pr.error.aborted, in both
binding-mcp-http.spec and binding-mcp-openapi.spec) that sends a real
non-2xx status begin before aborting, deterministically triggering
error-relay mode before the abort — confirmed failing 3/3 without the fix
and passing 5/5 with it.
Also fixes an unrelated oversight from an earlier commit in this branch:
mcp/create.pr.aborted/server.rpt (the peer self-consistency script for
McpServerIT, distinct from the real IT's http/create.pr.aborted/server.rpt)
never got the "write notify REQUEST_RECEIVED" its paired client.rpt was
updated to await, causing a deterministic 5s timeout in that peer test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
…handling Moves bodyTemplate, with.query, and the resources/read request path onto the same streaming JsonPipeline architecture already used for with.body, replacing the fully-buffered onMcpRequest fallback. McpToolsCallProxy and McpResourcesReadProxy each fully dispatch their own request shape from onMcpBegin/pumpRequest now, keeping the two kinds' streaming logic independent per their existing separation. New McpHttpQuery and McpHttpDiscard terminal JsonSink transforms capture a projected query object and discard a validate-only body respectively. Removes projectBuffer, runInto, projectInto, templateInto, validate, newValidator/newProjector/newTemplate, and queryStringFromBytes, which have no callers left once every request shape streams directly. Also fixes a decode-slot leak: McpProxy.onMcpData kept accumulating body bytes into a decode slot after a request was already dispatched (e.g. by McpResourcesReadProxy's now-eager onMcpBegin), leaking the slot forever since onMcpRequest never fires again to release it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactor the monolithic
McpProxyclass into a hierarchy of kind-specific proxy implementations to improve code clarity and maintainability.Changes
Architecture:
McpProxyfrom a concrete class to an abstract base class containing shared state and lifecycle methodsMcpHttpProxyabstract subclass for HTTP-backed kinds (tools/call, resources/read) that own theHttpProxydelegate and request/response streaming machineryMcpToolsListProxy— handles tools/list requests (buffered response)McpResourcesListProxy— handles resources/list requests (buffered response)McpPromptsListProxy— handles prompts/list requests (buffered response)McpToolsCallProxy— handles tools/call requests (HTTP-backed with optional streaming)McpResourcesReadProxy— handles resources/read requests (HTTP-backed with optional streaming)Refactoring details:
onMcpRequest()abstract method, overridden per kindonUpstreamResponse()abstract method for HTTP-backed kindsresponseBegin()andresponseStreamable()methods, overridden only byMcpResourcesReadProxynewStream()factory method to use a switch statement that instantiates the appropriate proxy typekindanddelegatethroughout the codebase; behavior is now determined by the concrete classBenefits:
kindanddelegatethroughout the coderesourceandkindfrom base class)Fixes #1976
https://claude.ai/code/session_01PjuRiuQF9f7Z4rSrsFXbWB