Address stephentoub review feedback on HTTP request callback support (+ cross-SDK parity)

[SteveSandersonMS]

Follow-up to #1689 addressing the review comments from @stephentoub. The bulk is .NET; a second commit propagates the applicable naming/efficiency changes to the other language SDKs so the CopilotRequestHandler shape stays consistent across all six. No behavior changes to the public callback model.

Implemented (12 comments)

CopilotWebSocketMessage

• Rename Text(...) factory to FromText(...).
• Remove the redundant Binary(...) factory; the public primary constructor CopilotWebSocketMessage(ReadOnlyMemory data, bool isBinary) already covers binary construction.
• GetText() decodes via Encoding.UTF8.GetString(Data.Span) instead of allocating with .ToArray().

CopilotRequestHandler

• New CopilotRequestHandler(HttpClient?) constructor so consumers can supply their own HttpClient, falling back to the shared process-wide instance.
• Forward the original HTTP method casing (drop ToUpperInvariant(); HttpMethod comparison is already case-insensitive).

Allocation/perf on the hot paths

• Send WebSocket frames through the ReadOnlyMemory overload (no ToArray()).
• Receive WebSocket frames through the Memory overload and rent the receive buffer from ArrayPool.
• Write request-body chunks via buffer.Write(chunk.Span).
• Don't pass the pump's cancellation token to Task.Run itself, so the pump still runs its cleanup if the linked token is already cancelled.

TFM cleanup

• Drop the #if NETSTANDARD2_0 branches in StreamResponseAsync in favour of new netstandard2.0 polyfills in DownlevelExtensions.cs: Encoding.GetString(ReadOnlySpan), HttpContent.ReadAsStreamAsync(CancellationToken), Stream.Write(ReadOnlySpan), and WebSocket SendAsync(ReadOnlyMemory)/ReceiveAsync(Memory) plus a ValueWebSocketReceiveResult shim. On net8/net10 the native BCL methods are used; call sites are now TFM-uniform.
• The ns2.0 HttpContent.ReadAsStreamAsync(CancellationToken) polyfill now honors the token (throws if already cancelled before the tokenless BCL call).

Cross-SDK parity (Go, Rust, Java)

Since these APIs are unshipped, the same renames/cleanups land in the other SDKs using each language's idiom — no two SDKs carry a redundant binary-message factory anymore:

• Go: drop the redundant per-chunk copy in streamResponseToSink (the sink's writeText already copies eagerly via string(...)); remove the redundant NewBinaryMessage factory (exported Data/Binary fields make a struct literal sufficient). NewTextMessage kept — New* is the idiomatic Go constructor name.
• Rust: rename text -> from_text; remove the redundant binary associated fn (the pub data/pub binary fields suffice); construct binary frames via a struct literal.
• Java: rename static text -> fromText; remove the redundant static binary factory (the record's canonical constructor suffices).

Node.js and Python are unaffected — they have no message wrapper type (raw string | Uint8Array / str | bytes) and their forwarding paths are already copy-light.

Two further per-frame copy removals, internal-only (no public API change):

• Rust: send_request_message now moves the owned payload into String::from_utf8 (zero-copy on valid UTF-8 — the common case for LLM traffic), falling back to lossy decoding only on invalid bytes, instead of always allocating via from_utf8_lossy().into_owned().
• Java: a new internal LlmInferenceExchange.writeResponseBinary(byte[], offset, length) overload base64-encodes a subrange directly, so streamResponse no longer allocates + arraycopies an exact-length frame per response chunk. The single-arg overload is kept for the WebSocket bridge.

Validation

• .NET builds clean on net8.0 / net10.0 / netstandard2.0 (TreatWarningsAsErrors, 0 warnings); CopilotRequest* e2e pass on net8.0 (5/5) and net472 (4/4, exercises the ns2.0 polyfill assembly at runtime).
• Go: go build + e2e green. Rust: build + cargo fmt --check + clippy -D warnings clean, e2e 4/4. Java: compiles clean (JDK 25).

Not adopting the BCL WebSocket types (responding to @stephentoub)

System.Net.WebSockets.WebSocket as the base class. WebSocket models one endpoint of one connection: SendAsync writes to the peer, ReceiveAsync reads from the peer. Our handler is a mediator on a relay with two directions to two different parties:

// What a consumer overrides today — direction is explicit, both sides mutable:
public override Task SendRequestMessageAsync(CopilotWebSocketMessage m)  // runtime -> upstream
public override Task SendResponseMessageAsync(CopilotWebSocketMessage m) // upstream -> runtime

A WebSocket subclass has a single SendAsync/ReceiveAsync with no notion of which side a frame belongs to, so the relay can't be expressed cleanly. It's also pull-based (the consumer owns a receive loop with buffer management and fragment reassembly), whereas we hand the consumer a fully-assembled CopilotWebSocketMessage and pump for them. Deriving from WebSocket would also force implementations of State, CloseStatus, SubProtocol, Abort, CloseOutputAsync, etc. — members with no meaning for a relay hook — and the Memory overloads are core-only, reintroducing the netstandard2.0 split this PR just removed.

WebSocketCloseStatus (the enum). It can't replace CopilotWebSocketCloseStatus, which carries Exception? Error and string? ErrorCode and is mapped to the runtime via end-of-stream / error-with-code — never an RFC numeric close code. Adding it alongside what we already have would only change the close code sent to the upstream provider, which providers don't act on, so it solves no consumer problem. A consumer who genuinely needed a specific upstream close can already override CloseAsync.

Deferred — needs maintainer input (not in this PR)

• Rpc.cs (generated types) — a question about why generated wire types surface; not a code change.
• Client.cs runtimeShutdownCompleted removal / graceful-shutdown wait reasoning — these question the shutdown redesign that landed in HTTP request callback support #1689; reverting would be a significant change, so flagging rather than acting.

[Copilot]

Pull request overview

Follow-up to #1689 for the .NET SDK to address review feedback around HTTP request callback support, focusing on API cleanup, allocation/perf improvements on WebSocket and streaming paths, and consolidating netstandard2.0 compatibility via polyfills.

Changes:

• Refines WebSocket message APIs and reduces allocations (span-based UTF-8 decode, ReadOnlyMemory send, ArrayPool-based receive buffering).
• Adds an injectable HttpClient option to CopilotRequestHandler while keeping a shared default.
• Replaces conditional compilation in call sites with netstandard2.0 polyfills (Encoding/Stream/WebSocket/HttpContent shims).

Show a summary per file

File	Description
dotnet/src/Polyfills/DownlevelExtensions.cs	Adds netstandard2.0 polyfills for Encoding span decode, HttpContent ReadAsStreamAsync(CancellationToken), Stream span write, and WebSocket Memory-based send/receive + receive result shim.
dotnet/src/CopilotRequestHandler.cs	Updates WebSocket message factories/decoding, optimizes send/receive paths, introduces injectable HttpClient, and removes TFM-specific branches in streaming code paths.

Copilot's findings

Comments suppressed due to low confidence (1)

dotnet/src/CopilotRequestHandler.cs:503

• hasBody uses reference inequality (method != HttpMethod.Get/Head). Since method is always a new HttpMethod, this comparison will never match and GET/HEAD requests will be treated as having a body (and may end up with a synthetic empty Content when adding content headers). Use HttpMethod.Get.Equals(method) / HttpMethod.Head.Equals(method) (or compare the method string ordinal-ignore-case) instead.

        var method = new HttpMethod(exchange.Method);
        var message = new HttpRequestMessage(method, exchange.Context.Url);

        var hasBody = method != HttpMethod.Get && method != HttpMethod.Head;
        var body = await DrainAsync(exchange.RequestBody).ConfigureAwait(false);
        if (hasBody && body.Length > 0)

• Files reviewed: 2/2 changed files
• Comments generated: 1

[github-actions]

Cross-SDK Consistency Review ✅

This PR maintains strong cross-SDK consistency. Here's a summary of what was verified:

CopilotWebSocketMessage API cleanup (applied to all 4 affected SDKs)

SDK	Old factory	New factory	Binary removal
.NET	Text(string)	FromText(string)	✅ Binary(ReadOnlyMemory<byte>) removed
Go	—	NewTextMessage kept (idiomatic New*)	✅ NewBinaryMessage removed
Java	text(String)	fromText(String)	✅ binary(byte[]) removed
Rust	text(impl Into<String>)	from_text(impl Into<String>)	✅ binary(Vec<u8>) removed

All renames follow each language's naming idiom (PascalCase/camelCase/snake_case) — consistent by design, not identical by accident.

Node.js and Python correctly omitted

Confirmed both SDKs use raw string | Uint8Array / str | bytes for WebSocket messages — no wrapper type exists, so there's nothing to rename or remove.

Custom HTTP client supply — already consistent across all SDKs

The new .NET constructor injection (CopilotRequestHandler(HttpClient?)) is .NET-idiomatic. All other SDKs already have their own idiomatic mechanism:

• Go: Transport http.RoundTripper field
• Java: protected HttpClient httpClient() override
• Node.js / Python / Rust: sendRequest / send_request method override

No gaps here.

No stale references found

Searched docs, samples, and tests — no remaining calls to the removed Binary/NewBinaryMessage/binary/binary factories or the old Text/text names.

All changes are internally consistent and no other SDKs need follow-up updates from this PR.

Generated by SDK Consistency Review Agent for issue #1775 · sonnet46 2.2M · ◷